Fixed #7041 - Toggle size effect demo not working. #203

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

kzys commented May 5, 2011

Size effect's problems are

  • Don't swap from/to on "show" mode.
  • Modify some properties with setTransition, but don't save/restore these properties.

In this branch, I've fixed these problems to close #7041.

Effects (size): Size effect should swap from/to on show, and restore …
…more properties. Fixed #7041 - Toggle size effect demo not working.

Why did you remove this? This removes the default values if you are using a 'show' / 'hide' toggle, and its not a part of the size effect

Owner

kzys replied May 5, 2011

Sorry, but original scale effect don't work smoothly on my Safari 5 and Firefox 4. I'll move this to new branch.

gnarf replied May 5, 2011

File a new ticket on it? :)

I feel like this should probably take into account that the default for a show should be {height:0, width:0} -> original and then the default for a hide should be original -> {height:0, width:0}

Owner

kzys replied May 5, 2011

Thank you. I'll fix too.

I would like to suggest if ( o.mode === "toggle" && mode === "hide" ) {

Also don't forget the whitespace, preferring double quotes and === where possible... http://docs.jquery.com/JQuery_Core_Style_Guidelines

Also - I tried to start a discussion of which direction should actually swap on the ticket, chime in people? http://bugs.jqueryui.com/ticket/7041#comment:4

Owner

kzys replied May 5, 2011

Thank you. I'll fix this.

Owner

gnarf commented May 5, 2011

Also, you might wanna make the changes from this commit to see what happens in another "toggle" scenario.

Effects (size): Code cleanup.
 * Separete `scale` effect's problem.
 * Clearify the default value of `from` and `to`.
 * Follow http://docs.jquery.com/JQuery_Core_Style_Guidelines.
Contributor

kzys commented May 5, 2011

Thank you Corey. I've added new commit to clean up my code. Please review again.

I'm still not sure if this should be mode === "show" or mode === "hide"

Other than this question, it looks good to me!

Owner

kzys replied May 5, 2011

I think it should be mode === "show" because the default is { height: 0, width: 0 } -> original.

gnarf replied May 5, 2011

After having seen the way it reads in the "visual effects test" page updates I've been doing, try doing something like this:

$( "#sizeToggle" ).bind( "click", function() {
    var opts = { to: { width: 300, height: 300 } },
                wait = 500, duration = 2000;
    $( this ).addClass( "current" )
        .toggle( "size", opts, duration )
        .delay( wait )
        .toggle( "size", opts, duration, function() {
            $( this ).removeClass( "current" );
        });
});

It makes pefect sense there (and looks pretty decent!!!) because the element starts out shown, but what if the element started hidden? Which case do we anticipate being more popular? With the toggle case, using mode === "show" in the if, it would make the options effectively hide to and hide from. Which again, seems to make sense (if the element started shown)

I just realized while I was playing around with this commit that there are still two cases to handle here... || ( mode === "hide" ? original : { height: 0, width: 0 } ) and the line below

Also, since we seem to be using this {height:0, width:0} default here a lot, can we assign it to a var?

gnarf replied May 5, 2011

In particular try doing a .hide("size", 2000).show("size", 2000) as well as .toggle("size", 2000).toggle("size", 2000) on an element...

gnarf replied May 5, 2011

How does this look to you?

// assuming zero = { height: 0, width: 0 };
el.from = o.from || ( mode === "show" ? zero : original );
el.to = o.to || ( mode === "hide" ? zero : original );`

This even covers the effect version to just default to original for both.

Contributor

kzys commented May 5, 2011

Thank you! I've merged your suggestion.

Owner

gnarf commented May 5, 2011

@kzys sorry if I didn't explain myself fully, that first half of the if was still totally valid, that was just a replacement for the else case... Also, could you maybe move that zero var definition up into the var statement?

Contributor

kzys commented May 5, 2011

Finally I understand your point.

On #sizeToggle of "visual effects test" page (It's very helpful. Thank you), First toggle hide element and second toggle show element again. And second animation looks odd because it's zero -> original.

However, It's problem of show, not toggle. We should swap from/to on show because other effect do this. For example, $(shown).hide('slide', { direction: 'left' }); is slide to left, but $(hidden).show('slide', { direction: 'left' }); is slide from left.

Owner

gnarf commented May 7, 2011

I think I'm sold on the reverse direction for show... All the time... Because of your point about the directions on slide/etc. We really need to document that behavior though...

Owner

gnarf commented May 10, 2011

For right now, can you put the if back in that works for toggle show only? I'd like to at least get this change in to fix this bug for the toggle case... We can further discuss the fate of the rest of show animations for size later...

Contributor

kzys commented May 12, 2011

Like this?

Owner

gnarf commented May 12, 2011

Not quite, you still need the direction detection in the "else" :

if ( o.mode === "toggle" && mode === "show" ) {
    el.from = o.to || zero;
    el.to = o.from || original;
} else {
    el.from = o.from || ( mode === "show" ? zero : original );
    el.to = o.to || ( mode === "hide" ? zero : original );
}
Contributor

kzys commented May 12, 2011

Oh, Sorry. I've fixed (actually just a copy-and-paste...).

Owner

gnarf commented Jun 10, 2011

This no longer wants to merge cleanly, could you rebase / merge with changes upstream?

Also, make sure to cover the whitespace issues :)

Owner

gnarf commented Jun 21, 2011

There have been a lot of changes upstream, I'd like to keep this fix, if you want to re-make it on top of a clean master that would be awesome. Please re-open / make a new pull when you get this mergable.

@gnarf gnarf closed this Jun 21, 2011

Contributor

kzys commented Jun 22, 2011

Okay.

gnarf added a commit that referenced this pull request Oct 24, 2012

kborchers added a commit that referenced this pull request Nov 1, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment