Effects: Split, new effect #185

Closed
wants to merge 40 commits into
from

Conversation

Projects
None yet
7 participants
@jstroem

jstroem commented Apr 18, 2011

The split effects reported in feature bug http://bugs.jqueryui.com/ticket/7069 has been implemented

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

This does have a dependency on effects.core.js doesn't it?

This does have a dependency on effects.core.js doesn't it?

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Mar 27, 2011

Owner

ofc!

Owner

jstroem replied Mar 27, 2011

ofc!

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

The other UI effect that is similar also takes a "pieces" argument, and uses Math.sqrt(pieces) to determine rows/cols - I like passing rows/cols but perhaps we should also support pieces?

The other UI effect that is similar also takes a "pieces" argument, and uses Math.sqrt(pieces) to determine rows/cols - I like passing rows/cols but perhaps we should also support pieces?

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

If you were to call this effect on multiple elements, this o is going to overwrite the passed in o

If you were to call this effect on multiple elements, this o is going to overwrite the passed in o

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

Take a look at $.effects.setMode( el, o.mode ) -- it converts "toggle" to "show" or "hide" for you.

Take a look at $.effects.setMode( el, o.mode ) -- it converts "toggle" to "show" or "hide" for you.

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

There is a $.effects.createWrapper( el ) that might help reduce the need to make your own container here.

There is a $.effects.createWrapper( el ) that might help reduce the need to make your own container here.

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

You might want to look at the other effects for the $.effects.save( el, props ) calls - it caches "starting" css values you can restore with restore...

Also, you are going to want to el.show() before trying to get .offset() and width, etc...

You might want to look at the other effects for the $.effects.save( el, props ) calls - it caches "starting" css values you can restore with restore...

Also, you are going to want to el.show() before trying to get .offset() and width, etc...

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 26, 2011

You can replace this whole test with:

 effect("#build", "build", { easing: 'linear' });

You can replace this whole test with:

 effect("#build", "build", { easing: 'linear' });

Jesper Lindstroem Nielsen added some commits Mar 27, 2011

Jesper Lindstroem Nielsen Jesper Lindstroem Nielsen
Made some bugs so now build works propperly.
Still considering using $.effects.createWrapper and
$.effects.save/restore.
@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 28, 2011

Just being super picky - But this text should probably just be "pinweel" :)

Just being super picky - But this text should probably just be "pinweel" :)

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

All the others default to random: false - why default to random here?

All the others default to random: false - why default to random here?

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

Is there any reason to not put this in startSplitAnim()

Is there any reason to not put this in startSplitAnim()

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

missing spaces .css( 'visibility', 'hidden' )

missing spaces .css( 'visibility', 'hidden' )

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

Store [ 'visibility', 'opacity' ] as modifiedProps or something, and reuse it for the restore call later...

Store [ 'visibility', 'opacity' ] as modifiedProps or something, and reuse it for the restore call later...

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

Spacing between the () and the args

Spacing between the () and the args

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

Also Whitespace between () and args

Also Whitespace between () and args

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

Also - currently the effects api helpers generally take ( el, otherParams ) - might be worth passing the el from startSplitAnim in rather than redeclaring - if we ever decide to use the jQuery.sub() idea I have out there, we can move it over...

Also - currently the effects api helpers generally take ( el, otherParams ) - might be worth passing the el from startSplitAnim in rather than redeclaring - if we ever decide to use the jQuery.sub() idea I have out there, we can move it over...

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

I'm not sure you need i and j - couldn't you do:

for ( left = 0; left < width; left += pieceWidth ) { 
   for ( top = 0; top < height; top += pieceHeight ) {
      /// work...
   }
}

Maybe even left = -Math.floor( ( pieceWidth * columns - width ) / 2); so that the pieces will be more "centered" and not lose all the excess transparent area on the right and bottom

I'm not sure you need i and j - couldn't you do:

for ( left = 0; left < width; left += pieceWidth ) { 
   for ( top = 0; top < height; top += pieceHeight ) {
      /// work...
   }
}

Maybe even left = -Math.floor( ( pieceWidth * columns - width ) / 2); so that the pieces will be more "centered" and not lose all the excess transparent area on the right and bottom

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Mar 29, 2011

you can bring the { up to this line: var opt = $.extend({

you can bring the { up to this line: var opt = $.extend({

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

Awesome work here @Lindstroem! So, this brings up one bit of conversation - This effect set totally includes (and supersedes) the current effects.explode - I'm thinking we might want to replace the current explode effect with yours...

Owner

gnarf commented Apr 18, 2011

Awesome work here @Lindstroem! So, this brings up one bit of conversation - This effect set totally includes (and supersedes) the current effects.explode - I'm thinking we might want to replace the current explode effect with yours...

tests/visual/effects.all.html
@@ -186,7 +187,42 @@
<p>hide/show (jQuery)</p>
</div>
</li>
+
+ <li>

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

Since this doesn't really do anything "visual" do you think you could clear it out of this test? If its not working, the next 4 effects aren't going to work either...

@gnarf

gnarf Apr 18, 2011

Owner

Since this doesn't really do anything "visual" do you think you could clear it out of this test? If its not working, the next 4 effects aren't going to work either...

ui/jquery.effects.split.js
+
+ //Helper function to control the split on each animation
+ function startSplitAnim( el, o, animation, next ) {
+ //Support for pieces

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

Style Guidance: empty line before comments...

@gnarf

gnarf Apr 18, 2011

Owner

Style Guidance: empty line before comments...

ui/jquery.effects.split.js
+ //Sets mode for toggle
+ opt.mode = $.effects.setMode( el, opt.mode );
+
+ opt.show = 0 + ( opt.mode == 'show' );

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

don't need the 0 +, assuming you just want opt.show = opt.mode === "show"

@gnarf

gnarf Apr 18, 2011

Owner

don't need the 0 +, assuming you just want opt.show = opt.mode === "show"

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

Also, I just got called out on some of my code for jquery.color -- We should always prefer using double quotes instead of single quotes, as well as === instead of == ( unless we really want the == )

Owner

gnarf commented Apr 18, 2011

Also, I just got called out on some of my code for jquery.color -- We should always prefer using double quotes instead of single quotes, as well as === instead of == ( unless we really want the == )

ui/jquery.effects.split.js
+ //Sets mode for toggle
+ opt.mode = $.effects.setMode( el, opt.mode );
+
+ opt.show = 0 + ( opt.mode == 'show' );

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2011

Owner

Having just seen the above 20 lines of code duplicated, might it be possible to create a function to DRY this up a little?

function splitOptions( el, defaults, o ) {
   var opt = $.extend( {}, defaults, o );
   if ( el.is( ':hidden' ) && opt.mode === "toggle") {
     opt.reverse = !opt.reverse;
   }
   opt.mode = $.effects.setMode( el, opt.mode );
   opt.show = opt.mode === "show";
   return opt;
 }

 // then in the functions:
 var el = $( this ),
     opt = splitOptions( el, {.....}, o );
@gnarf

gnarf Apr 18, 2011

Owner

Having just seen the above 20 lines of code duplicated, might it be possible to create a function to DRY this up a little?

function splitOptions( el, defaults, o ) {
   var opt = $.extend( {}, defaults, o );
   if ( el.is( ':hidden' ) && opt.mode === "toggle") {
     opt.reverse = !opt.reverse;
   }
   opt.mode = $.effects.setMode( el, opt.mode );
   opt.show = opt.mode === "show";
   return opt;
 }

 // then in the functions:
 var el = $( this ),
     opt = splitOptions( el, {.....}, o );
@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem May 16, 2011

Done!

I have another idea to a feature that could be implemented;
it could be nice if you could (as a option) define the way the effect is happening, like in effects.blockfade where it is comming from top-left and going to bottom-right.
But i don't have any really good idea to implement this?

jstroem commented May 16, 2011

Done!

I have another idea to a feature that could be implemented;
it could be nice if you could (as a option) define the way the effect is happening, like in effects.blockfade where it is comming from top-left and going to bottom-right.
But i don't have any really good idea to implement this?

Jesper Lindstroem Nielsen added some commits May 17, 2011

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Jun 22, 2011

Owner

RE your old comment about blockfade direction, we could use an option like origin: "left top", but it would need to support top/center/bottom left/center/right -- Take a look at the UI Position arguments on http://docs.jquery.com/UI/API/1.8/Position

Owner

gnarf commented Jun 22, 2011

RE your old comment about blockfade direction, we could use an option like origin: "left top", but it would need to support top/center/bottom left/center/right -- Take a look at the UI Position arguments on http://docs.jquery.com/UI/API/1.8/Position

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Jun 28, 2011

Owner

There was an update to the core functions that requires updating this branch. -- The .queue() portion is handled in effects.core now.

Owner

gnarf commented Jun 28, 2011

There was an update to the core functions that requires updating this branch. -- The .queue() portion is handled in effects.core now.

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Oct 12, 2011

Owner

This pull has gone stale, any chance we can get it freshened up?

Owner

gnarf commented Oct 12, 2011

This pull has gone stale, any chance we can get it freshened up?

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Oct 12, 2011

so there is really 2 things to this pull the first is the split thing just on normal elements and, as far as i know, there is nothing more to put into this? it works right?

then there is the effects on the text witch i have made some of but as i remember have some problems with. So i will write this on my todo list and i should have time se whats needed!

jstroem commented Oct 12, 2011

so there is really 2 things to this pull the first is the split thing just on normal elements and, as far as i know, there is nothing more to put into this? it works right?

then there is the effects on the text witch i have made some of but as i remember have some problems with. So i will write this on my todo list and i should have time se whats needed!

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Oct 12, 2011

ohh.. just looked at the repo..
this is only a pull on the split things on the html elements not the text split?

I think i stopped working on the text split effects because there were some guys (on weekly dev meetings) that said this split wasn't going to get under the hood of jquery ui?

but i don't know if that has changed now where 2.0 is a little closer?

jstroem commented Oct 12, 2011

ohh.. just looked at the repo..
this is only a pull on the split things on the html elements not the text split?

I think i stopped working on the text split effects because there were some guys (on weekly dev meetings) that said this split wasn't going to get under the hood of jquery ui?

but i don't know if that has changed now where 2.0 is a little closer?

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Oct 12, 2011

Owner

We can look again at the text split, but I still think this could use some freshening up... I'd like to see the split effects land in 1.9, but It no longer merges cleanly with master, and I believe its still using an API that isn't current in 1.9... track me down, talk with me about it :)

Owner

gnarf commented Oct 12, 2011

We can look again at the text split, but I still think this could use some freshening up... I'd like to see the split effects land in 1.9, but It no longer merges cleanly with master, and I believe its still using an API that isn't current in 1.9... track me down, talk with me about it :)

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Oct 12, 2011

i just gonna take a quick look at the code tomorrow and then i will catch you at irc and we can discuss whats need to be done!

jstroem commented Oct 12, 2011

i just gonna take a quick look at the code tomorrow and then i will catch you at irc and we can discuss whats need to be done!

@paulirish

This comment has been minimized.

Show comment Hide comment
@paulirish

paulirish Nov 9, 2011

Member

@Lindstroem can you summarize what the status is? I'd love to see this make its way in and can ask Scott to take a look..

Member

paulirish commented Nov 9, 2011

@Lindstroem can you summarize what the status is? I'd love to see this make its way in and can ask Scott to take a look..

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Nov 9, 2011

Owner

I'm still willing to update this branch myself if @Lindstroem doesn't have the time for it

Owner

gnarf commented Nov 9, 2011

I'm still willing to update this branch myself if @Lindstroem doesn't have the time for it

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Nov 9, 2011

Yea. so I've been looking into it today and it is good to go.. but the merging with master fails as i understand from @gnarf37.. so if there is something there needs to be updated in the code just notifiy me!

jstroem commented Nov 9, 2011

Yea. so I've been looking into it today and it is good to go.. but the merging with master fails as i understand from @gnarf37.. so if there is something there needs to be updated in the code just notifiy me!

@jstroem jstroem closed this Nov 9, 2011

@paulirish

This comment has been minimized.

Show comment Hide comment
@paulirish

paulirish Nov 9, 2011

Member

lindstroem can you reopen ;)

Member

paulirish commented Nov 9, 2011

lindstroem can you reopen ;)

@jstroem jstroem reopened this Nov 9, 2011

Jesper Lindstroem Nielsen added some commits Nov 9, 2011

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Feb 8, 2012

@gnarf37: any news on this?

jstroem commented Feb 8, 2012

@gnarf37: any news on this?

@Jellyfrog

This comment has been minimized.

Show comment Hide comment
@Jellyfrog

Jellyfrog Apr 17, 2012

Bump

Bump

@paulirish

This comment has been minimized.

Show comment Hide comment
@paulirish

paulirish Apr 17, 2012

Member

👍

Member

paulirish commented Apr 17, 2012

👍

@sindresorhus

This comment has been minimized.

Show comment Hide comment
@sindresorhus

sindresorhus Apr 17, 2012

Contributor

👍

Contributor

sindresorhus commented Apr 17, 2012

👍

@jstroem

This comment has been minimized.

Show comment Hide comment
@jstroem

jstroem Apr 18, 2012

been looking into this; in the pull request status: http://oksoclap.com/ui-pull-requests the following is pointed out:

#185 - Split

  • on hold, waiting for a big block of time to review new set of effects
  • would rather get existing code cleaned up first

i don't know if anyone is up for reviewing and how the status is for the other cleaning tasks. I will try to be at the weekly meeting for UI today and ask into this.

jstroem commented Apr 18, 2012

been looking into this; in the pull request status: http://oksoclap.com/ui-pull-requests the following is pointed out:

#185 - Split

  • on hold, waiting for a big block of time to review new set of effects
  • would rather get existing code cleaned up first

i don't know if anyone is up for reviewing and how the status is for the other cleaning tasks. I will try to be at the weekly meeting for UI today and ask into this.

@gnarf

This comment has been minimized.

Show comment Hide comment
@gnarf

gnarf Apr 18, 2012

Owner

We haven't used that clap in a while, but that was the last time we did a big group review of pulls.

I do want this to land in 1.9 soon, but I'm pretty involved in some other areas of the project right now that stop me from doing the necessary work at the current moment.

Owner

gnarf commented Apr 18, 2012

We haven't used that clap in a while, but that was the last time we did a big group review of pulls.

I do want this to land in 1.9 soon, but I'm pretty involved in some other areas of the project right now that stop me from doing the necessary work at the current moment.

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Nov 10, 2012

Member

@gnarf37, @scottgonzalez, @jzaefferer ping.

Member

mikesherov commented Nov 10, 2012

@gnarf37, @scottgonzalez, @jzaefferer ping.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Nov 12, 2012

Owner

We're still waiting until we've cleaned up more of the existing code before adding more effects.

Owner

scottgonzalez commented Nov 12, 2012

We're still waiting until we've cleaned up more of the existing code before adding more effects.

@mikesherov

This comment has been minimized.

Show comment Hide comment
@mikesherov

mikesherov Sep 12, 2014

Member

Thanks again for the had work here, but we've finally decided that we're not going to pursue further effects at this time, and therefore, are closing this pull request.

Member

mikesherov commented Sep 12, 2014

Thanks again for the had work here, but we've finally decided that we're not going to pursue further effects at this time, and therefore, are closing this pull request.

@mikesherov mikesherov closed this Sep 12, 2014

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