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

option to adjust documentElement #821

Closed

Conversation

Projects
None yet
2 participants
@SassNinja
Copy link

commented Sep 14, 2017

I've had a great problem with the noUiSlider when using it inside of an Off-Canvas component (foundation-sites framework) with the option contentScroll:false.
Precisely the slider didn't move on touchmove.

After digging through the noUiSlider sources I've found out the reason:
the slider is listening on scope_DocumentElement what is always the whole document. But since the Off-Canvas is preventing all touch on the body the slider doesn't work anymore.

To fix this you have to change scope_DocumentElement from document to some element in the Off-Canvas. Unfortunately there's hasn't been an option to adjust that variable.

That's why I've made this PR which provides an option to adjust it.
You can either pass an element or a string with value target (will internally use scope_Targetthen). Fallback is document so 100% backwards compatibility is guaranteed.

@leongersen would you please review and merge if it's ok for you?

SassNinja added some commits Sep 12, 2017

Add option to adjust scope_DocumentElement
This option lets the user change the DocumentElement if the default value (whole document) can't be used.
It's possible to either set 'target' (what will use the internal scope_Target) or a custom element.
Fallback is the whole document.
@leongersen

This comment has been minimized.

Copy link
Owner

commented Dec 29, 2017

This looks good. I'm really sorry for not replying earlier; I've been travelling for a while. I'll soon push some changes to this pull request to add some comments detailing why some changes were made and remove the distributed files. Thanks!

@SassNinja

This comment has been minimized.

Copy link
Author

commented Jan 3, 2018

Thanks for your response!
I'm glad you hear it looks good for you on the whole and am looking forward to seeing this merged (after your changes).

@SassNinja

This comment has been minimized.

Copy link
Author

commented Apr 3, 2018

@leongersen I just came back to this feature because I need it for a new project and would like to avoid to adjust my vendors locally again.

Do you have time to finish & merge this PR?
If I may help with anything just let me know

@leongersen

This comment has been minimized.

Copy link
Owner

commented Apr 12, 2018

Sorry I never got back to this! I am not sure about an option to replace the document element, as a lot of events (e.g. tap) have to use it to determine where the slider was touched. I'd be OK merging this if cases like that would not have any issues. If you could set up a test page (JSfiddle or the like), I'd be happy to get this in this week.

Merge branch 'dev' into feature/option-document-element
# Conflicts:
#	distribute/nouislider.css
#	distribute/nouislider.js
#	distribute/nouislider.min.css
#	distribute/nouislider.min.js
#	src/js/options.js
@SassNinja

This comment has been minimized.

Copy link
Author

commented Apr 23, 2018

@leongersen the new option does not affect any existing project and is meant to be used in cases where the default documentElement does not work (e.g. when using the Off-Canvas plugin of the Foundation framework as mentioned above).

Here's a simple codepen to show the problem and how it's fixed using the new option.
https://codepen.io/KaiTheGreat/pen/GdoZGd

I've merged dev into my feature branch so my PR is (again) ready to merge.
Would you like me to do anything else?

@SassNinja

This comment has been minimized.

Copy link
Author

commented Jul 4, 2018

stackoverflow.com/questions/51006807/nouislider-touch-events-not-working-in-foundation-off-canvas-menu

Any comment from you beside the link would be helpful.
What was your intention to post it?

The mentioned 'bugfix' on stackoverflow is no fix because the option in talk (contentScroll) has a right to exist! In my case I can't disable this option.

The only solution so far is the additional option, introduced by this PR.

@leongersen leongersen closed this Sep 12, 2018

@leongersen leongersen added this to the noUiSlider 12 milestone Sep 13, 2018

@leongersen leongersen changed the base branch from dev to master Sep 13, 2018

@leongersen leongersen reopened this Sep 13, 2018

Repository owner deleted a comment from SassNinja Sep 13, 2018

@leongersen leongersen self-assigned this Sep 13, 2018

@leongersen

This comment has been minimized.

Copy link
Owner

commented Sep 14, 2018

I just released noUiSlider 12.0.0, which adds this option. I did not include the "target" string option, as the same result can be achieved by passing the original slider element to the documentElement function.

@leongersen leongersen closed this Sep 14, 2018

@mmarkelov mmarkelov referenced this pull request Dec 21, 2018

Closed

Implement extra nouislider v12 features #3

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.