#7060 closed enhancement (fixed)

Opened March 03, 2011 07:11PM UTC

Closed March 11, 2011 02:30PM UTC

Improving internal Effects API

Reported by: gnarf Owned by: gnarf
Priority: blocker Milestone: 1.9.0
Component: ui.effects.core Version: 1.8.10
Keywords: Cc:
Blocked by: Blocking:
Description

Currently, the effects functions are being passed an object that looks something like this:

 { options: { pieces: 9 }, mode: 'hide', callback: fn, duration: 200 }

It seems that the 

options
here becomes a little redundant internally, causing a need to reference 
 o.options.pieces
instead of just 
 o.pieces

We can pretty easily override this function to create an "Effect Options Object" that would look more like this: 

 { effect: 'explode', pieces: 9, mode: 'hide', complete: fn, duration: 200 }

There will be a BC break here - Any custom 

 $.effects[]
functions that have been written will need to be updated to replace 
 o.options.*
-> 
 o.*
and 
 o.callback
-> 
 o.complete

It would be possible to add a BC duck punch if necessary, however my quick google search shows no results for "jquery ui effects custom".

I've started cleaning up the API a little here: https://github.com/gnarf37/jquery-ui/compare/master...effects-api

Let me know if you think the BC punch is needed - I still want to try and clean up some of the effects.js files before I file a pull...

Attachments (0)
Change History (13)

Changed March 03, 2011 07:13PM UTC by scottgonzalez comment:1

priority: minor → blocker
status: new → open

Changed March 07, 2011 10:06PM UTC by gnarf comment:2

https://github.com/jquery/jquery-ui/pull/141

Changed March 08, 2011 08:05AM UTC by gnarf comment:3

Adding another internal effects API issue / idea onto this ticket - might be worthy of its own.

In thinking about how to handle the BC shim for this API change, and other ways we might clean up the internal API - I thought we could move the actual effect implementation functions out of the 

 $.effects[]
and into 
 $.effects.effect[]
. It gives us a discreet namespace where we are sure that the function is an effect API function. Currently, other functions exist out on 
 $.effects[]
that aren't effects, allowing you to get interesting results for 
 $("div").toggle("animateClass")
- Letting that namespace hold utility functions, while pushing the actual effects into a new namespace could show some minor improvements.

Also - We could potentially leverage 

 $.effects.$ = jQuery.sub()
and extend its 
fn
to serve up the setMode, save, restore, createWrapper, removeWrapper, and a few other helper functions that all take 
 el
as a first parameter. Each of the effect functions can then use 
 $.effects.$( this )
and have all the helper functions available in a nice chain.

Got most of these changes in and you can look at the branch here: https://github.com/gnarf37/jquery-ui/compare/effects-api...effects-sub -- it seems github is mistakenly showing some of the whitespace changes that were in the effects-api branch on the effects-sub branch, but you should get the idea...

Changed March 08, 2011 10:15AM UTC by gnarf comment:4

blocking: → 7069

Changed March 08, 2011 10:52AM UTC by gnarf comment:5

owner: → gnarf
status: open → assigned

Changed March 08, 2011 10:58AM UTC by gnarf comment:6

blocking: 7069 → 6022, 7069

Changed March 08, 2011 11:01AM UTC by gnarf comment:7

blocking: 6022, 7069 → 6022, 6781, 7069

Changed March 09, 2011 03:18PM UTC by scottgonzalez comment:8

resolution: → fixed
status: assigned → closed

Merged in gnarf's branch.

Changed March 10, 2011 11:13PM UTC by gnarf comment:9

resolution: fixed
status: closed → reopened

There is still https://github.com/jquery/jquery-ui/pull/146 to look at scott - gonna reopen this until you decide there too :)

Changed March 11, 2011 04:20AM UTC by gnarf comment:10

blocking: 6022, 6781, 7069 → 6781, 7069

(In #6022) https://github.com/jquery/jquery-ui/pull/149

Changed March 11, 2011 02:30PM UTC by gnarf comment:11

resolution: → fixed
status: reopened → closed

Reclosing at scotts request... see http://bugs.jqueryui.com/ticket/7103 for the other parts...

Changed May 25, 2011 05:26PM UTC by gnarf comment:12

blocking: 6781, 7069 → 7069

Changed November 12, 2012 03:45AM UTC by mikesherov comment:13

blocking: 7069