Separating configuration from initialization of the jQuery animations and general moving things around to reduce function nesting and function length.

CommentFileSizeAuthor
#27 1685140_collapsible_fieldset_refactor_27.patch7.67 KBseutje
#24 1685140_collapsible_fieldset_refactor_24.patch7.67 KBfrega
#23 1685140_collapsible_fieldset_refactor_23.patch7.67 KBfrega
#22 1685140_collapsible_fieldset_refactor_22.patch7.63 KBfrega
#20 1685140_collapsible_fieldset_refactor_20.patch7.06 KBfrega
#18 1685140_collapsible_fieldset_refactor_18.patch7.15 KBfrega
#18 1685140_collapsible_fieldset_refactor_18_interdiff.txt866 bytesfrega
#16 1685140_collapsible_fieldset_refactor_16.patch7.17 KBseutje
#16 1685140_collapsible_fieldset_refactor_16.interdiff.txt857 bytesseutje
#15 1685140_collapsible_fieldset_refactor_15.patch7.16 KBfrega
#12 1685140_collapsible_fieldset_refactor_11.patch7.96 KBfrega
#11 1685140_collapsible_fieldset_refactor_11.patch7.96 KBfrega
#9 1685140_collapsible_fieldset_refactor_9.patch7.92 KBfrega
#8 1685140_collapsible_fieldset_refactor_8.patch7.92 KBethanw
#7 1685140_collapsible_fieldset_refactor_7.patch7.92 KBfrega
#6 1685140_collapsible_fieldset_refactor_6.patch7.94 KBfrega
#4 1685140_collapsible_fieldset_refactor_2.patch7.35 KBfrega
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Related: #209420: Remove the scrolling animation in collapsible.js

frega’s picture

frega CreditAttribution: frega commented
Assigned: Unassigned » frega

assigning for dcmuc code sprint

frega’s picture

frega CreditAttribution: frega commented
Assigned: frega » Unassigned
Status: Active » Needs review

This is an *initial* stab at refactoring this. Created a CollapsibleFieldset-object/wrapper for each instance on the page (à la verticaltab). Not finished yet, but @nod_ is this what you intended in terms of refactoring?

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_2.patch7.35 KB

This is an *initial* stab at refactoring this. Created a CollapsibleFieldset-object/wrapper for each instance on the page (à la verticaltab). Not finished yet, but @nod_ is this what you intended in terms of refactoring?

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

yeah that's a good start :)

the created fieldset should be stored somewhere (don't take example on Drupal.ajax, the new tableheader patch is better to look at)
there are some semicolons missing, run your code though jshint early on as well :)

the setup and toggle method could probably be simplified
whenever you have an object for options, try to add that to an accesible thing somewhere in Drupal.autocomplete or drupal.settings

frega’s picture

frega CreditAttribution: frega commented
Status: Needs work » Needs review
FileSize
1685140_collapsible_fieldset_refactor_6.patch7.94 KB

Continued on refactoring in the patch. Further splitting up into separate methods.

Re: Drupal.settings - introducing Drupal.settings.collapsibleFieldset (optional).

Todo/Questions:
1) Do we want to scrollIntoView at every step of the animation, do we want to scrollIntoView at all?
2) Settings per fieldset?

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_7.patch7.92 KB

Oops, missed a superfluous var self = this; in the constructor.

ethanw’s picture

ethanw CreditAttribution: ethanw commented
FileSize
1685140_collapsible_fieldset_refactor_8.patch7.92 KB

Reviewed patch for functionality, all okay.

Make two slight changes, as discussed at the sprint: use jQuery.extend for Drupal.settings parameters, as they were initially used for settings arguments and removed a `that` var where jQuery proxy had been used instead.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_9.patch7.92 KB

Rerolled last patch because git/jshint were nagging about trailing whitespace ;)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

Just a few changes and it should be good:

you can move the content of the setup function inside the constructor directly,
Create a Drupal.CollapsibleFieldset.fieldsets = []; where you'll put the objects you create in the behavior. Have a look at tableheader to see what I mean.
separate the .once() call and the actual loop over the results, that'll help later on.

Nice work so far :)

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_11.patch7.96 KB

Rerolled according to #10.

Notes:
1) Do we want to scrollIntoView at every step of the animation, do we want to scrollIntoView at all?
2) Do we need individual settings per fieldset (i.e. passing those to the constructor?)

PS. This is a great example where backbonejs would help us organize and standardize "representation" and "state" in the client in a clean and consistent fashion :)

frega’s picture

frega CreditAttribution: frega commented
Status: Needs work » Needs review
FileSize
1685140_collapsible_fieldset_refactor_11.patch7.96 KB

Missed setting to "needs review", sorry. Notes etc. still applies.

seutje’s picture

seutje CreditAttribution: seutje commented

scrollIntoView should be gone soon: #209420: Remove the scrolling animation in collapsible.js will probably break this partially. Considering it's a rather small change and it's been UX approved, maybe we should just incorporate it into this refactoring?

frega’s picture

frega CreditAttribution: frega commented

I am more than happy to remove that stuff - @nod_ any opinions?

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_15.patch7.16 KB

After rereading #209420: Remove the scrolling animation in collapsible.js (seems pretty unanimous), I rerolled the patch removing the scrollIntoView-part (saves 800 bytes, yay!).

seutje’s picture

seutje CreditAttribution: seutje commented
FileSize
1685140_collapsible_fieldset_refactor_16.interdiff.txt857 bytes
1685140_collapsible_fieldset_refactor_16.patch7.17 KB

It makes sense to send the settings to the constructor, otherwise any settings passed to the attach handler are simply ignored. And it seems nice to have the ability to have separate settings per collapsible fieldset.

Attached patch removes some whitespace and passes the settings to the constructor.
Also: I learned how to make interdiffs :D

seutje’s picture

seutje CreditAttribution: seutje commented

Also gave this a quick manual test on IE8 just to make sure, seems to work as expected.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_18_interdiff.txt866 bytes
1685140_collapsible_fieldset_refactor_18.patch7.15 KB

Fixed small bug in #16 pushing the settings-var onto the Drupal.CollapsibleFieldset.fieldsets-array rather than providing it to the constructor. Changed the constructor so that it does not merge all of Drupal.settings into the local settings-property.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Needs work

in setupSummary there is

this.$node.
  bind()
  .trigger();

it should be 

this.$node
  .bind()
  .trigger();

You can get rid of the default varible in the constructor, the object can be the first parameter for $.extend()

onLegendclick, e.preventDefault() should be first (in calse the toggle crashes for some reason). and you shouldn't return anything.

The slidedown configuration object in toggle() could be nice to have as an object member in case we want to change those settings. 

      Drupal.CollapsibleFieldset.fieldsets = Drupal.CollapsibleFieldset.fieldsets || [];

That line don't need to be in attach, would be better right after the constructor is defined (and without the || check, just Drupal.CollapsibleFieldset.fieldsets = [];)

in the attach still just send settings.collapsibleFieldset as the second parameter of the constructor.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_20.patch7.06 KB

Rerolled patch as per #19.

I didn't quite understand this bit: "The slidedown configuration object in toggle() could be nice to have as an object member in case we want to change those settings." - You would you like to be able to inject other options (or override others?). $.slideDown afaik only takes duration, easing and complete, but I might be wrong :)

frega’s picture

frega CreditAttribution: frega commented
Status: Needs work » Needs review

Missed the state change again, sorry.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_22.patch7.63 KB

Refactored as per IRC to be more similar to tableheader (i.e. Drupal.CollapsibleFieldset = CollapsibleFieldset;) allowing for better minification; settings will be passed to slideUp/slideDown-animation functions, new: additional events "completeSlideDown" and "completeSlideUp" to allow for custom stuff.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_23.patch7.67 KB

Fixed coding-standard, and refactor $.each to for-loop.

frega’s picture

frega CreditAttribution: frega commented
FileSize
1685140_collapsible_fieldset_refactor_24.patch7.67 KB

Missing space "onLegendClick"

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

Works great, thanks for sticking up with the painful review process :D

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Reviewed & tested by the community » Needs work

another patch broke this one, need reroll.

seutje’s picture

seutje CreditAttribution: seutje commented
Status: Needs work » Needs review
FileSize
1685140_collapsible_fieldset_refactor_27.patch7.67 KB

reroll, it was the removal of return false and addition of e.preventDefault() that broke it, but those changes were already incorporated in this one.

seutje’s picture

seutje CreditAttribution: seutje commented

adding reference to what broke this patch: #1749782: Use event.preventDefault(); and event.stopPropagation(); instead of return false;

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

All good now. Back to RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

In general I think I follow what's going on here, and seems like reasonable clean-up.

Committed and pushed to 8.x. Thanks!

Kiphaas7’s picture

Kiphaas7 CreditAttribution: Kiphaas7 commented

BONUS: Investigate if adding basic events and detach makes sense, as described in #1763812: [META] Provide complete attach/detach with basic events.

I think it does make sense to trigger some basic 'collapse initialized/removed' along with a detach.

Automatically closed -- issue fixed for 2 weeks with no activity.