While I'm rambling on and on about the Toggle function... I think there is another issue (though it might be a feature) with the code as it stands. If you unbind a toggle function, and then rebind it later, then the DOM element still has the "lastToggle" expando - so it remembers which step it was on when it was unbound.
If this is a feature, then it has an issue that if you re-bind a toggle handler to an event with less functions than you originally had, the modulus operation (this.lastToggle = ( this.lastToggle || 0 ) % i;) would put the current toggle index at an arbitrary position. I personally think it makes more sense to add "this.lastToggle = 0" (or $(this).data( 'lastToggle' + fn.guid, 0 ) with the patch I submitted) before the click handler is returned: this way the toggle cycle always resets to 0 when you bind. But that would break the functionality for anyone who expects it to work as it does now. Also, if that's not a problem, then perhaps the toggle function is a good candidate for a special event - so the data can be appended and cleaned up nicely? Earle On Sep 21, 12:23 pm, Mr Speaker <mrspea...@gmail.com> wrote: > I submitted a ticket and patch here:http://dev.jquery.com/ticket/5274 > > I had updated the patch to use a closure, instead of the data object, > like: > toggle: function( fn ) { > // Save reference to arguments for access in closure > var args = arguments, i = 1, lastToggle = 0; > ... > return this.click( jQuery.event.proxy( fn, function( event ) { > lastToggle = lastToggle % i; > ... > return args[ lastToggle++ ].apply( this, arguments ) || false; > })); > > }, > > But when testing the patch in my code I realised there was an issue > that was also present in the bindsequence demo: if you apply the code > to two different elements (eg, in the bindsequence example, if you > duplicate the #testId span with a different ID), each of them will > increment the internal count - the state is shared. I suppose this > could be a "feature" in that either trigger will increment the count, > but I don't think it would be expected behaviour. > > The function guid looks to me like the only thing that is unique per > element, per handler. It seems a bit funky, but it works: You can see > it in action here:http://www.mrspeaker.net/dev/jq/toggle-patched.html > - a link at the bottom of the page shows the same test with jQuery > nightly build. > > Earle > > On Sep 20, 7:29 pm, Michael Geary <m...@mg.to> wrote: > > > Hi Earle, I don't think I said that the toggle method isn't broken. It > > sounds like it is broken, since one would expect that it should be able to > > handle multiple simultaneous toggles on the same element like you are doing. > > > I just don't see why it uses data in the DOM element, when the closure is so > > much more straightforward. I guess it's just to make the unbinding work. > > Unless I misunderstand the code and comments, it sounds like the purpose is > > to let you do this: > > > $('#foo').toggle( fn1, fn2, fn3 ); > > > $('#foo').unbind( 'click', fn1 /* or fn2 or fn3, take your pick */ ); > > > That seems like ugly code to me, though. I have never liked the business of > > unbinding an event handler by passing the function reference to unbind, > > since it doesn't work with anonymous functions like the ones you typically > > use with bind or toggle. > > > Maybe I missed the point of how toggle is implemented, though. > > > -Mike > > > On Sat, Sep 19, 2009 at 7:58 PM, Mr Speaker <mrspea...@gmail.com> wrote: > > > > Hey Michael, > > > > Are you saying that the Toggle functionality isn't broken? I thought > > > that Toggle was supposed to work the way your bindsequence and my > > > dodgy patch worked? > > > > I like your namespaced event handler approach, and agree about saving > > > state in the DOM (I updated my patch to use .data functionality, which > > > is a bit nicer) but that's how it's done in Toggle in the core now. I > > > was just trying to leave the code as similar to the original as > > > possible. > > > > If Toggle is working correctly then I'll happily add my own custom > > > functions to work how I need it, but perhaps the documentation should > > > be updated to say "adding multiple toggle events to one element > > > does..." Actually, if it's working as intended then I'm not sure what > > > it does ;) > > > > Earle > > > > On Sep 20, 3:54 am, Michael Geary <m...@mg.to> wrote: > > > > You know, this is the kind of thing you can write yourself in just a few > > > > lines of code, without having to save state in the DOM (which I find > > > > distasteful). Here's a standalone version that doesn't do anything > > > > tricky > > > > like that. The only state it keeps is right there in the function > > > > itself. > > > > > While we're at it, we'll make it more general so it can sequence > > > callbacks > > > > for any event, and we'll give it a better name. (toggle specifically > > > implies > > > > *two* states, not many.) > > > > > $.fn.bindSequence = function( name ) { > > > > var args = arguments, i = 1; > > > > this.bind( name, function( event ) { > > > > args[i].call( this, event ); > > > > if( ++i >= args.length ) i = 1; > > > > }); > > > > return this; > > > > > }; > > > > > You can use it like this: > > > > > $('.testClass').bindSequence( 'click', > > > > function() { console.log( this.className + ' X' ); }, > > > > function() { console.log( this.className + ' Y' ); }, > > > > function() { console.log( this.className + ' Z' ); } > > > > ); > > > > > Looking at the toggle() source code, I see that it does some extra work > > > > having to do with unbinding. But you don't really need that either. > > > > Since > > > > bindSequence() lets you pass in the event name, you can bind and unbind > > > like > > > > this: > > > > > $('.testClass').bindSequence( 'click.mysequence', > > > > ... > > > > ); > > > > > $('.testClass').unbind( 'click.mysequence' ); > > > > > Here's a test page: > > > > >http://mg.to/jquery/bindsequence.html > > > > > Try the "click me" button several times, and then click the "unbind" > > > button > > > > and go back and click the "click me" button again. > > > > > Now you don't have to patch jQuery! :-) > > > > > -Mike > > > > > On Fri, Sep 18, 2009 at 6:40 PM, Mr Speaker <mrspea...@gmail.com> wrote: > > > > > > I recently deadline forced me to make a change to the jQuery core to > > > > > fix an issue I was having. Now that I'm refactoring the code, I'd like > > > > > to know if I've missed something obvious, or if there's a better way > > > > > to do it. I couldn't find any reference to this issue on the forums or > > > > > bug tracker. > > > > > > <div id="btnClose" class="icon">Close</div> > > > > > > $(".icon").toggle( > > > > > function(){ $(this).removeClass('two2').addClass('two1') }, > > > > > function(){ $(this).removeClass('two1').addClass('two2') } > > > > > ); > > > > > > $("#btnClose").toggle( > > > > > function(){ $(this).removeClass('one2 one3').addClass('one1'); > > > }, > > > > > function(){ $(this).removeClass('one1 one3').addClass('one2'); > > > }, > > > > > function(){ $(this).removeClass('one1 one2').addClass('one3'); > > > > > } > > > > > ); > > > > > > This doesn't work with jQuery (stable or nightly), as internally the > > > > > current toggle state is stored on the DOM node itself as "lastToggle" > > > > > and so increments multiple times per click. > > > > > > I had to modify the return statement of the toggle function so that I > > > > > could use the proxy guid as part of the counter identifier: > > > > > > var proxy = jQuery.event.proxy( fn, function(event) { > > > > > // Figure out which function to execute > > > > > this[ 'lastToggle' + proxy.guid ] = ( this[ 'lastToggle' + > > > > > proxy.guid ] || 0 ) % i; > > > > > > // Make sure that clicks stop > > > > > event.preventDefault(); > > > > > > // and execute the function > > > > > return args[ this[ 'lastToggle' + proxy.guid ]++ ].apply( this, > > > > > arguments ) || false; > > > > > }) > > > > > return this.click( proxy ); > > > > > > Which appears to work perfectly (and got me through my deadline ;) but > > > > > I'm sure isn't the most elegant way to do it. Am I even supposed to be > > > > > able to add multiple toggle event handlers? > > > > > > Thanks! > > > > > > Earle > > --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "jQuery Development" group. To post to this group, send email to jquery-dev@googlegroups.com To unsubscribe from this group, send email to jquery-dev+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/jquery-dev?hl=en -~----------~----~----~----~------~----~------~--~---
