Opened 10 years ago

Closed 10 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 10 years ago.
dojox_Calendar_AMD.diff (74.3 KB) - added by dylan 10 years ago.
First draft, dojox/widget/Calendar AMD conversion (incomplete)
dojox_Calendar_AMD_v2.diff (74.3 KB) - added by dylan 10 years ago.
Adds PEM's noted missing declare require statement
dojox_Calendar_AMD_testfixes.diff (11.1 KB) - added by Nick Fenwick 10 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 10 years ago.
cleaned up: test_DateTextBox.html, test_Calendar.html and test_CalendarViews.html

Download all attachments as: .zip

Change History (52)

Changed 10 years ago by Martin Repta

Attachment: calendarAMD.patch added

comment:1 Changed 10 years ago by bill

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

comment:2 Changed 10 years ago by dylan

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

comment:3 Changed 10 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 10 years ago by dylan

Attachment: dojox_Calendar_AMD.diff added

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

comment:4 Changed 10 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 10 years ago by dylan

Attachment: dojox_Calendar_AMD_v2.diff added

Adds PEM's noted missing declare require statement

comment:5 Changed 10 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 10 years ago by PEM

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

comment:7 Changed 10 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 10 years ago by dylan

In [28264]:

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

comment:9 Changed 10 years ago by dylan

Patch applied to trunk. Issues that remain:

  • Update tests
  • Convert CalendarFX
  • Actually test things

comment:10 Changed 10 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 10 years ago by bill

In [28312]:

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

comment:12 Changed 10 years ago by Nick Fenwick

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

comment:13 Changed 10 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 10 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 10 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 10 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 10 years ago by dylan

Ok, will review and commit asap.

comment:17 Changed 10 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 10 years ago by dylan

In [28753]:

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

comment:19 Changed 10 years ago by dylan

In [28771]:

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

comment:20 Changed 10 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 10 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 10 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 10 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 10 years ago by dylan

Thanks Doug, I'll work on that next.

comment:25 Changed 10 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 10 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 10 years ago by dylan

In [28870]:

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

comment:28 Changed 10 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 10 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 10 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 10 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 10 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 10 years ago by dylan

In [28896]:

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

comment:34 Changed 10 years ago by dylan

In [28897]:

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

comment:35 Changed 10 years ago by dylan

In [28898]:

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

comment:36 Changed 10 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 10 years ago by ben hockey

In [28930]:

remove trailing comma. refs #14711

comment:38 Changed 10 years ago by ben hockey

In [29062]:

trailing comma. refs #14711

comment:39 Changed 10 years ago by ben hockey

In [29063]:

trailing comma. refs #14711

comment:40 Changed 10 years ago by dylan

In [29131]:

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

comment:41 Changed 10 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 10 years ago by ben hockey

In [29147]:

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

comment:43 Changed 10 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 10 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 10 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 10 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 10 years ago by dylan

In [29328]:

refs #14711, clean up tests

comment:47 Changed 10 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.