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

feat(slider): customizable event selectors #2330

Draft
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

lubber-de
Copy link
Member

@lubber-de lubber-de commented Apr 19, 2022

Description

This PR makes the event selector target customizable via settings, so the initial idea of #1987 would be now easier doable based on the new codebase from #2327

@mhthies Please review, i am interested in your opinion

Testcase

https://jsfiddle.net/lubber/k3efcLuj/1/
https://fomantic-ui.com/jsfiddle/#!lubber/k3efcLuj/1/ (for mobile device testing)

Replaces

#1987

@lubber-de lubber-de added type/feat Any feature requests or improvements lang/javascript Anything involving JavaScript state/awaiting-reviews Pull requests which are waiting for reviews labels Apr 19, 2022
@lubber-de lubber-de added this to the 2.9.0 milestone Apr 19, 2022
@lubber-de lubber-de added the state/awaiting-docs Pull requests which need doc changes/additions label Apr 19, 2022
@mhthies
Copy link
Contributor

mhthies commented Apr 19, 2022

I like the idea! Unfortunately, this doesn't work as expected. Here is an extended JSFiddle with the code from this PR showing the different options:

The mouse setting does not have an effect due to the additional mousedown binding in line 249. I think, we can drop one of the two event bindings (but didn't test it yet).

Setting touchTarget to anything other than .thumb makes (very strange) problems due to my code, which assumes that the event target is always a thumb (see line 332). This section would need to be extended to use module.determine.closestThumb() just like the mouse down event handler does.

@lubber-de
Copy link
Member Author

Thanks for taking a look!

The mouse setting does not have an effect due to the additional mousedown binding in line 249. I think, we can drop one of the two event bindings (but didn't test it yet).

oh, yes, the second one seems to override the first (which is taking care of event bubbling...)
So i guess the second one can be removed ? Yes, have to test this

Setting touchTarget to anything other than .thumb makes (very strange) problems due to my code, which assumes that the event target is always a thumb (see line 332). This section would need to be extended to use module.determine.closestThumb() just like the mouse down event handler does.

Makes sense 👍🏼 I'll test this. Thanks for the hint!

@lubber-de lubber-de modified the milestones: 2.9.0, 2.9.x Sep 14, 2022
@lubber-de lubber-de marked this pull request as draft January 8, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/javascript Anything involving JavaScript state/awaiting-docs Pull requests which need doc changes/additions state/awaiting-reviews Pull requests which are waiting for reviews type/feat Any feature requests or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants