Opened 7 years ago

Closed 7 years ago

#14711 closed enhancement (fixed)

[patch] [cla] rewrite of dojox.Widget.Calendar into AMD

Reported by: Martin Repta Owned by: dylan
Priority: blocker Milestone: 1.8
Component: DojoX Widgets Version: 1.7.1
Keywords: Cc:
Blocked By: Blocking:

Description

I have rewritten Calendar widget into AMD syntax.
Originally there were two major files Calendar.js and CalendarViews?.js but they contained declares for all types of calendar. I had to move them into separate files to support AMD. This affects also way how to programmaticaly create calendar. The main difference is that you will always need to require widget for calendar that you want to create. It is the same as with widgets from dijit.

require(["dojo/ready",
 	"dojox/widget/MonthAndYearlyCalendar"
 ], function(ready, MonthAndYearlyCalendar) {
   ready(function() {  
      var cal_1 = new dojox.widget.MonthAndYearlyCalendar({}, dojo.byId("cal_1"));
   });
});

Patch file attached.

Attachments (5)

calendarAMD.patch (70.5 KB) - added by Martin Repta 7 years ago.
dojox_Calendar_AMD.diff (74.3 KB) - added by dylan 7 years ago.
First draft, dojox/widget/Calendar AMD conversion (incomplete)
dojox_Calendar_AMD_v2.diff (74.3 KB) - added by dylan 7 years ago.
Adds PEM's noted missing declare require statement
dojox_Calendar_AMD_testfixes.diff (11.1 KB) - added by Nick Fenwick 7 years ago.
dojox_Calendar_testfixes.diff, a git diff of my changes, patches on top of the latest commits to trunk. dojox.profile.js untouched.
tests.patch (5.3 KB) - added by André Ribeiro de Miranda 7 years ago.
cleaned up: test_DateTextBox.html, test_Calendar.html and test_CalendarViews.html

Download all attachments as: .zip

Change History (52)

Changed 7 years ago by Martin Repta

Attachment: calendarAMD.patch added

comment:1 Changed 7 years ago by bill

Summary: rewrite of dojox.Widget.Calendar into AMD[patch] [cla] rewrite of dojox.Widget.Calendar into AMD

comment:2 Changed 7 years ago by dylan

Milestone: tbd1.8
Owner: changed from dante to dylan
Priority: undecidedhigh
Status: newassigned

comment:3 Changed 7 years ago by dylan

I've started a new patch, but they are still many issues with this, namely:

  • Use of some old approaches (event)
  • Really really crufty tests which currently don't work
  • Some code that obviously won't work (typeof is not a function, it’s an operator, so the () around e.target._date isn't helpful, also if(d){date = date.add(date, "month", d);} won't work)
  • Some frankly ugly names for the various views, etc.

The patch is here in case someone has time to take this further.

Changed 7 years ago by dylan

Attachment: dojox_Calendar_AMD.diff added

First draft, dojox/widget/Calendar AMD conversion (incomplete)

comment:4 Changed 7 years ago by PEM

Dylan, there is a "dojo/_base/declare", missing in the define of _CalendarBase.js It also need to be added to the function params so that return declare know what "declare" is :)

Changed 7 years ago by dylan

Attachment: dojox_Calendar_AMD_v2.diff added

Adds PEM's noted missing declare require statement

comment:5 Changed 7 years ago by dylan

From my testing and PEM's testing, this replacement seems to work, but the tests are still broken. Does someone else want to tackle getting the test suite updated?

comment:6 Changed 7 years ago by PEM

It seems only CalendarFx? has not yet been converted to AMD, is it still in use or?

comment:7 Changed 7 years ago by dylan

I did not have time to convert it, if someone else wants to update the patch, I'm happy to review it. I'm going to go ahead and commit what's there.

comment:8 Changed 7 years ago by dylan

In [28264]:

update dojox/calendar to AMD, tests not yet updated, refs #14711

comment:9 Changed 7 years ago by dylan

Patch applied to trunk. Issues that remain:

  • Update tests
  • Convert CalendarFX
  • Actually test things

comment:10 Changed 7 years ago by Martin Repta

Please remove forgotten commas at the end of define, in these files:

_CalendarMonthView.js line 7

_CalendarMonthYearView.js line 11

comment:11 Changed 7 years ago by bill

In [28312]:

fix trailing commas, refs #14711 !strict, thanks Mangala.

comment:12 Changed 7 years ago by Nick Fenwick

None of these patches seem to modify dojox.profile.js to mark these modules as AMD?

comment:13 Changed 7 years ago by Nick Fenwick

I got around to pulling these patches and applying to 1.7 branch and testing, and found a few bugs. Attaching patch.

Outstanding issue: The fisheyelite fx calendar seems to run OK but doesn't actually animate. The FisheyeLite? is instantiated just fine but nothing seems to happen on mouseover. The highlight fx calendar works fine. No more time to investigate just now.

Changed 7 years ago by Nick Fenwick

dojox_Calendar_testfixes.diff, a git diff of my changes, patches on top of the latest commits to trunk. dojox.profile.js untouched.

comment:14 Changed 7 years ago by dylan

With the introduction of dojox/Calendar in 1.8, it may be more useful to just switch to using that since this code will be deprecated in favor of that module.

Would you prefer looking at that, or do you want me to merge this patch anyway?

comment:15 Changed 7 years ago by bill

I think they are different things:

  • dojox/Calendar is a schedule planner (like the Calendar application on a mac or iPad)
  • dojox/Calendar is just a date picker (like dijit/Calendar).

BTW dojox/Calendar doesn't load at all in trunk.

comment:16 Changed 7 years ago by dylan

Ok, will review and commit asap.

comment:17 Changed 7 years ago by dylan

In [28749]:

refs #14711, partial fix for AMD update for dojox/widget/Calendar, but the tests still don't pass, it's still using dojo/_base/event and inline dojo/connect for events in the test, and perhaps some other issues

comment:18 Changed 7 years ago by dylan

In [28753]:

refs #14711, start moving dojox/widget/Calendar away from connect to on

comment:19 Changed 7 years ago by dylan

In [28771]:

minor clean-up for dojox/widget/Calendar, refs #14711

comment:20 Changed 7 years ago by ben hockey

don't forget to update dojox/dojox.profile.js so that these converted files get tagged as amd. ie update the excludes array so that these module ids aren't going to match any more.

comment:21 Changed 7 years ago by dylan

In [28835]:

refs #14711, make sure dojox/widget/Calendar is tagged as AMD now that it's been converted

comment:22 Changed 7 years ago by dylan

[28855], [28856], [28857], [28858], and [28859] should now have this in decent shape. This needs some testing as a lot of things have moved around. I've tested in the latest FF, but haven't tried elsewhere yet.

Also note that the events for selecting month and year in the views that only show month and year don't seem to do much.

Also, console.log doesn't seem to be logging for the inline event handlers, haven't looked into that yet.

That said, it's pretty usable now, and should build more efficiently with smaller dependencies as well.

comment:23 Changed 7 years ago by Douglas Hays

DateTextBox.js needs to be split out into separate widgets and the popupClass widgets listed in their respective define() prereqs to make the day/month/year specific popups in test_DateTextBox.html work.

comment:24 Changed 7 years ago by dylan

Thanks Doug, I'll work on that next.

comment:25 Changed 7 years ago by dylan

Actually Doug, I don't follow, this is for dojox/Calendar. I don't see how this is related to dijit/DateTextBox?

comment:26 Changed 7 years ago by Douglas Hays

dojox/form/tests/test_DateTextBox.html has several failures now. The Day/Month/Year only boxes no longer work. The popupClass references in dojox/form/DateTextBox.js used to be brought in via dojox/widget/Calendar.js but not anymore.

comment:27 Changed 7 years ago by dylan

In [28870]:

refs #14711, missed one AMD conversion, some additional minor updates

comment:28 Changed 7 years ago by dylan

Just realized that dojox/widget/Calendar doesn't have a Claro theme implementation.

I get it now Dough, didn't realize there were tests in dojox/form related to this. Will look through those asap.

comment:29 Changed 7 years ago by dylan

In [28879]:

refs #14711, first pass at updating dojox/form/DateTimeBox to work with the newly AMD-ified dojox/widget/Calendar. This is currently broken, as the way the widgets are defined and inherit is complex, and so currently it's failing to be able to instantiate the base class... need to do some clean-up work still, but out of time until Sunday or next week. Checking it in, in case someone else wants to continue the clean-up effort between now and Sunday

comment:30 Changed 7 years ago by Colin Snover

Milestone: 1.82.0

1.8 is frozen. Move all enhancements to next release. If you need an exemption from the freeze for this ticket, contact me immediately.

comment:31 Changed 7 years ago by bill

Milestone: 2.01.8

The enhancement part is already done, the remaining work is to make dojox/form/DateTextBox work again (see above comments from Dylan and Doug).

comment:32 Changed 7 years ago by bill

Priority: highblocker

... so I suppose it should be marked as a blocker, given that it's now in a state of regression (ie, DateTextBox worked in 1.7 but it's broken in trunk).

comment:33 Changed 7 years ago by dylan

In [28896]:

fix issue with missing dependencies in dojox/widget/Calendar, refs #14711

comment:34 Changed 7 years ago by dylan

In [28897]:

update dojox/widget/Calendar tests to use dojo/aspect, refs #14711

comment:35 Changed 7 years ago by dylan

In [28898]:

minor updates to dojo/widget/Calendar, refs #14711

comment:36 Changed 7 years ago by dylan

In [28899]:

minor updates to dojo/widget/Calendar and dojox/form/DateTimeBox, still failing, but mixin order cleaned-up, refs #14711

comment:37 Changed 7 years ago by ben hockey

In [28930]:

remove trailing comma. refs #14711

comment:38 Changed 7 years ago by ben hockey

In [29062]:

trailing comma. refs #14711

comment:39 Changed 7 years ago by ben hockey

In [29063]:

trailing comma. refs #14711

comment:40 Changed 7 years ago by dylan

In [29131]:

still struggling to fix issues with AMD conversion, refs #14711

comment:41 Changed 7 years ago by dylan

I need help figuring this out, see my comment in the last commit with fixme's identifying the area that is failing. I believe I've managed to break the inheritance model of this with my AMD conversion, so the issue I'm seeing is likely a side effect of this... basically it cannot find the header or footer of the base widget from dojox/form or dijit/form that it's expecting in order to populate the header or footer.

comment:42 Changed 7 years ago by ben hockey

In [29147]:

some help with the dojox/widget/Calendar AMD conversion. refs #14711

comment:43 Changed 7 years ago by ben hockey

that should move you closer. dojox/form/tests/test_DateTextBox.html now loads without any obvious errors.

i'll leave it to you to see if there are any other loose ends - i know that dojox.widget.FisheyeLite? still needs to be converted to AMD and, i'm not sure it matters, but if you wanted to provide a little bit of backwards compatibility, you might have dojox/widget/Calendar, dojox/widget/CalendarFx and dojox/widget/CalendarViews import all the various modules that used to be declared in those modules.

comment:44 Changed 7 years ago by dylan

In [29236]:

refs #14711, port dojox/widget/FisheyeLite to AMD, a few tests need work due to reliance on old or hardcoded API

comment:45 Changed 7 years ago by dylan

Current issues (pretty close to having this all cleaned-up):

test_FisheyeLite.html test: inlined event handlers should be fixed, easeIn/easeOut of last test case, read the fine print event handlers need to be fixed, old style usage of query.instantiate using dojox.widget.FisheyeLite? syntax, add in stop event syntax

test_Calendar.html test: test/CalendarCustomFx example is failing to render (dojo/parser::parse() error TypeError?: query(theQuery, fromNode).onmouseover is not a function), constraints on fifth example in page are never being called, addFx, should query(theQuery, fromNode).onmouseover(function(evt){ be cleaned up

test_CalendarViews.html test: all tests pass in FF and Chrome

test_DateTextBox.html: all tests pass in FF and Chrome

Changed 7 years ago by André Ribeiro de Miranda

Attachment: tests.patch added

cleaned up: test_DateTextBox.html, test_Calendar.html and test_CalendarViews.html

comment:46 Changed 7 years ago by dylan

In [29328]:

refs #14711, clean up tests

comment:47 Changed 7 years ago by dylan

Resolution: fixed
Status: assignedclosed

Closing this out, opening a separate ticket for the more minor issues which I'll defer until after 1.8.

Note: See TracTickets for help on using tickets.