Opened 10 years ago

Closed 4 years ago

#9442 closed enhancement (wontfix)

nodeList-fx not in base

Reported by: nonken Owned by: dante
Priority: high Milestone: 2.0
Component: fx Version: 1.3.0
Keywords: NodeList-fx, base Cc: James Burke, Eugene Lazutkin
Blocked By: Blocking:

Description

Imho it is confusing to many people that you have to do a

dojo.require(dojo.NodeList?-fx)

to get additional fx functionality for NodeList?. Besides Dom querying and current nodelist features, it would be a big added value when fx functionality would be included in the base. Many people don't use this feature because they don't know it exists.

Attachments (1)

Nodelist-anim.patch (14.8 KB) - added by dante 9 years ago.
sigh. lots of whitespace from a "convert spaces to tabs" on file. apologies.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 10 years ago by dante

I don't agree NodeList?-fx should be in base. They are high-performance combined() animations,and would require pulling in the entire dojo.fx module into base (~2k) ... I think the addition of a single .animate() function wrapping animateProperty (and heavily documenting the speed implications of it) would be a better solution.

I'd be OK with putting in the one .animate() call from: http://code.google.com/p/plugd/source/browse/trunk/base.js#502

as is, but not much more on top of that. The Core FX are there, just not in NodeList? without the module in the interest of size. .animate doesn't conflict with the NodeList?-fx versions: .anim and .animateProperty

comment:2 Changed 10 years ago by nonken

If the overhead is too large, something like animate() would serve the same purpose of making animations on a nodelist simpler. So that would be a good enhancement instead.

comment:3 Changed 9 years ago by dante

Milestone: tbd1.5
Owner: changed from Bryan Forbes to dante

comment:4 Changed 9 years ago by dante

Cc: James Burke Eugene Lazutkin added

this just came up again, are either of you opposed to a new api called .animate in base NodeList that skips the requirement of calling require(nodelist-fx) for simple animations in base?

comment:5 Changed 9 years ago by James Burke

It would be good to see how big the change is, but I am hoping with elazutkin's nodelist helpers it is not that big. If it is a couple of lines, seems good to have. Although a bit weird with NodeList?-fx that we have an animate, animateProperty and anim on NodeList?.

comment:6 Changed 9 years ago by dante

the .animate code is already in plugd (see link). It is about 4 thin lines of code. It could be adjusted to be an _adaptAsForEach(dojo.anim) but that would cause an onEnd handler to be fired once for EACH node in the list rather than the way plugd has it, which is once based on the last animation.

I would prefer it to be a new API (.animate) over having it as .anim (in base only, which would be overwritten by nodelist-fx's .anim implementation) but that is only my initial reaction. Having a new API seems less like voodoo, where if you dojo.require(nodelist-fx) you get the performant version of anim() versus the one-line adaptedAsEach version. a new API would seem easier to document as the low-performance variation of anim(), but perhaps no one would notice (provided the API signatures and behavior were identical between nodelist-fx:anim and base-nodelist:anim, minus the performance).

comment:7 Changed 9 years ago by James Burke

Right, only connecting to the last anim for the onEnd is better.

It looks like the NodeList?-fx's anim returns the animation instead of plugd which returns the NodeList?, so it seems hard to reuse "anim" as the method name in NodeList?. Too bad. So I guess "animate" will have to be it. We can clean it up in the 2.0 timeframe.

comment:8 Changed 9 years ago by dante

There is nothing saying the anim() in plugd has to return only the NodeList? ... I actually created a patch for Nodelist-fx which will preserve backwards compat but allow you to pass auto:true as a kwArg part, which will auto-play the animation and return the nodelist. see: #8721

comment:9 Changed 9 years ago by James Burke

Trouble is I think the plugd version of returning this is more correct. :) But I guess if this is just a helper and we have to deal with NodList?-fx being loaded, we could match the NodeList?-fx's anim behavior and call it anim. That works.

comment:10 Changed 9 years ago by dante

Milestone: 1.51.6

comment:11 Changed 9 years ago by dante

two builds run, comparing byte addition:

adcsugr:release dante$ ls -la anim/dojo/dojo.js
-rw-r--r--  1 dante  staff  90433 Jul 18 11:28 anim/dojo/dojo.js
adcsugr:release dante$ ls -la orig/dojo/dojo.js
-rw-r--r--  1 dante  staff  90281 Jul 18 11:28 orig/dojo/dojo.js

from above patch. thoughts?

Changed 9 years ago by dante

Attachment: Nodelist-anim.patch added

sigh. lots of whitespace from a "convert spaces to tabs" on file. apologies.

comment:12 Changed 9 years ago by dante

w/ gzip:

adcsugr:release dante$ ls -la anim/dojo/dojo.js.gz 
-rw-r--r--  1 dante  staff  30775 Jul 18 11:28 anim/dojo/dojo.js.gz
adcsugr:release dante$ ls -la orig/dojo/dojo.js.gz 
-rw-r--r--  1 dante  staff  30718 Jul 18 11:28 orig/dojo/dojo.js.gz

I'm ok with this. +1 from core and I'll do up (read: steal from plugd) unit tests and commit.

comment:13 Changed 9 years ago by dante

a single animation's onEnd passes the node reference to the callback. eg:

dojo.anim(n, { opacity:1 }, 100, null, function(node){
    console.warn(n == node); // true
});

which would happen if you just passed the callback to connect:

d.connect(anim, "onEnd", cb); // no context, no curry

Could use the builtin connect/hitch to cause the callback to be scoped to the nodelist, but the node being passed to the function is nearly useless.

d.connect(anim, "onEnd", this, cb)

I chose d.partial(cb, this) because passing the list in place of the node seemed more logical for a list based animation. Side effect, the node of the last animation is passed in the second position:

query(".bar").animate({ opacity:1 }, 1000, null, null, function(list, lastNode){
     this.forEach(function(n){
          // i'm query('.bar')
     });
     this.at(-1)[0] == lastNode;

});

in case anyone is wondering.

comment:14 Changed 9 years ago by dante

the above references to this in the callback should be list

comment:15 Changed 8 years ago by dante

Milestone: 1.62.0

comment:16 Changed 4 years ago by dylan

Resolution: wontfix
Status: newclosed

Not really relevant for Dojo 2, and not being changed for 1.x, so closing.

Note: See TracTickets for help on using tickets.