Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix 327 - Responsive Settings via setOption #1334

Closed
wants to merge 1 commit into from

Conversation

simeydotme
Copy link
Collaborator

Fixes #327

setOption method.

Previously was impossible to set the responsive settings via setOption due
to:
1. Settings would never get merged, always would lose previous breakpoints
2. The responsive settings were registered at init, and not at reinit.

Added in a specific check for setOption and Responsive to merge new
responsive breakpoint options, and over-write existing ones.

Moved the breakpoint registration into it's own method and call it on init
and reInit. Also breakpoint registration will clear out duplicate
breakpoints.
@@ -1472,6 +1454,46 @@

};

Slick.prototype.registerBreakpoints = function() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you annotate this? Is it thoroughly tested? Explain what it does and the necessity.

@simeydotme
Copy link
Collaborator Author

Added some annotations, for you, @kenwheeler :)
#327 (comment)
^^ this comment has a pretty thorough JSFiddle attached, and I tested-the-mother-loving-shit out of it on my local machine. -- the commits belie the simplicity of the feature, really... the large chunks are just de-duping the _.options.responsive array and pushing new values :) -- the rest of it is just code-rearrangement and it was tested well. 😅

@kenwheeler
Copy link
Owner

Thank you so much for the annotations. I need to fiddle with this myself a bit before merging, cause this is a biggum. Well done.

@simeydotme
Copy link
Collaborator Author

Yeh absolutely mate! - cheers, and it would have been much simpler to de-dupe if the responsive JSON-style array was an indexed-object instead, but I had to work around that architecture 😙 ... Perhaps something to consider for v2 😄

@kenwheeler
Copy link
Owner

👍

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

Successfully merging this pull request may close these issues.

"Responsive" option isn't being updated in slickSetOption
2 participants