Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#13699 closed enhancement (fixed)

Convert dojo/fx to AMD with minimal deps

Reported by: Chris Mitchell Owned by: Chris Mitchell
Priority: high Milestone: 1.7
Component: Core Version: 1.6.1
Keywords: Cc: Colin Snover
Blocked By: Blocking:

Description

Convert dojo/fx to AMD with minimal deps

Change History (13)

comment:1 Changed 8 years ago by Chris Mitchell

Resolution: → fixed
Status: new → closed

In [26235]:

fixes #13699 update dojo/fx to AMD with minimal deps

comment:2 Changed 8 years ago by Colin Snover

This patch violates JavaScript (& Dojo) code conventions. Uppercase first letters are reserved for constructors (dom, array, etc. are not constructors) and uppercase letters in other places in variables are reserved for delineating word breaks in variable names (i.e. thisDomThing not thisDOMThing). Please fix at your earliest convenience. Thanks!

Also, the return value change doesn’t make sense; it should be building an object and assigning the object to dojo.fx and returning the same object, otherwise people can’t hot patch properly.

Finally as I mentioned in another ticket, as pointed out by kgf, your change to remove “dojo.” from the comment on line 20 probably breaks the doc parser.

Last edited 8 years ago by Colin Snover (previous) (diff)

comment:3 Changed 8 years ago by Chris Mitchell

In [26240]:

refs #13699 change parms to follow code conventions in dojo.fx

comment:4 Changed 8 years ago by Chris Mitchell

In [26241]:

refs #13699 fix api doc in dojo.fx

comment:5 Changed 8 years ago by Chris Mitchell

In [26242]:

refs #13699 make return values overridable in dojo.fx

comment:6 Changed 8 years ago by Chris Mitchell

Pretty sure I've addressed the above issues. Please review and let me know if there is anything else you see. Thanks!

comment:7 Changed 8 years ago by Chris Mitchell

Cc: Colin Snover added

comment:8 Changed 8 years ago by Chris Mitchell

In [26245]:

refs #13699 typo in AMD conversion of dojo.fx

comment:9 Changed 8 years ago by bill

I'm still seeing hundreds of references to dojox in dojo/fx. I guess that's not covered by this ticket?

Oops, nevermind, I was looking at dojox/fx and this is about dojo/fx.js.

Last edited 8 years ago by bill (previous) (diff)

comment:10 Changed 8 years ago by Chris Mitchell

I've got a good start at dojox.fx conversion locally. Once I get past the last few probs in gfx3d, I'll continue with the modules under dojox/fx. -Chris

comment:11 Changed 8 years ago by bill

Priority: normal → high
Resolution: fixed
Status: closed → reopened

After these changes the Toggler test (in fx.html) is failing. Ran test on IE8.

comment:12 Changed 8 years ago by Chris Mitchell

Resolution: → fixed
Status: reopened → closed

I typo'd the ticket # on the patch for Toggler. It went in against #13688 r26266. Toggler and easing tests should all be working as of that fix.

comment:13 Changed 8 years ago by Chris Mitchell

In [26276]:

refs #13699 AMD bugfixes for easing and base/fx

Note: See TracTickets for help on using tickets.