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
-~----------~----~----~----~------~----~------~--~---
