ded / bonzo

library agnostic, extensible DOM utility
Other
1.32k stars 137 forks source link

safer typechecking in .toggle() and .show() #91

Closed ryanve closed 12 years ago

ryanve commented 12 years ago

In .toggle() I added a this.length check so that the optional callback does not get called for empty sets, and I made it so that the callback only gets called if its typeof is function. This makes it easier to interchange jQuery and bonzo b/c the .toggle(duration) syntax can now be used w/o throwing an error:

.toggle(null, 'block') // specify a display type w/o supplying a callback
.toggle(47) // same effect as .toggle()
.toggle(47, 'easeInBounce') // same effect as .toggle()

Also (a consideration) does it make sense to pass the collection as the scope to the toggle callback?

rvagg commented 12 years ago

soooo... I'd be happy to pull this (unless @ded pipes up) cause I'm cool with type checking (could be done in more places!), but the one thing I want to question is why bypass the callback for empty sets? I'm not really sure what the purpose of the callback is in the absence of jQuery-style animations except for jQuery-compatibility; and I would have assumed that jQuery would still trigger the callback even for empty sets, no? And yes, a .call(this) is cool too IMO.

ryanve commented 12 years ago

@rvagg jQuery.fn.toggle calls the callback once for each elem in the collection (the current elem => this) whereas currently bonzo calls the callback exactly once (at the end, outside of the loop, after all the elems have been toggled). Was that an intentional simplification? Omit the length check if you want—I just couldn't think of a practical reason why someone would need the callback called for an empty set. I can imagine some uses for the callback beyond animations tho, like maybe loading assets that are only needed if part of the page was toggled open.

rvagg commented 12 years ago

I don't know why the callback is there, apart from jQuery compatibility, which is why I'm reluctant to change too much about it without @ded's input.

ded commented 12 years ago

the callback isn't needed, but since we have it, it should mirror the style of jq wrt each element receiving a callback

rvagg commented 12 years ago

btw, you need to edit src/bean.js and check that in rather than ./bean.js which gets written over each time (don't check it or bean.min.js in either). I'll manually fix this time but remember it for future PRs. Cheers.

ryanve commented 12 years ago

@rvagg Ok cool thanks / gotcha =]