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

Slider: onRelease sometimes doesn't fire #2695

Closed
clawisan opened this issue May 13, 2019 · 5 comments · Fixed by #5359
Closed

Slider: onRelease sometimes doesn't fire #2695

clawisan opened this issue May 13, 2019 · 5 comments · Fixed by #5359

Comments

@clawisan
Copy link

Detailed description

When changing the slider value, the onRelease event doesn't fire sometimes. This happens inconsistently, but usually every 20 or so attempts. I can reproduce this by starting at any position on the slider (0-100%), dragging, and finally releasing at any other point on the slider, but it seems to happen more often when I release at 0%.

My expectation is that every time I release the handle, onRelease is fired and should be the last event to be fired (after all the onChanges that occurs during the drag).

Component version: [email protected] and the version currently running on Carbon Storybook

Browser: Chrome 74.0.3729.108 (Official Build) (64-bit)

Steps to reproduce the issue

  1. In the slider component in Storybook http://react.carbondesignsystem.com/?selectedKind=Slider&selectedStory=default&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel
  2. Drag the handle from anywhere on the slider (eg. 50%) to a different position (eg. 0%)
  3. Release the handle

Additional information

Video reproducing the problem in Storybook
carbonslider_onrelease.mp4.zip

Would it be possible to get a fix for v9 urgently as we need to meet our timeline for May?

@carbon-bot carbon-bot transferred this issue from carbon-design-system/carbon-components-react May 13, 2019
@carbon-bot carbon-bot added package: react carbon-components-react type: bug 🐛 labels May 13, 2019
@carbon-bot
Copy link
Contributor

Hi there! 👋 If you're wondering why this issue was moved, we're currently updating our repo structure so that every package is found in the same project.

This should not have any impact for you, but we wanted to give you a heads up in case you were wondering what is going on. If you have any questions, feel free to reach out to us on Slack or contact us at: [email protected]. Thanks!

@mattrosno
Copy link
Member

@dakahn can you make sure this issue makes it to the correct triage project?

@dakahn dakahn added version: 9 Issues pertaining to legacy Carbon priority: high severity: 2 https://ibm.biz/carbon-severity status: help wanted 👐 labels May 14, 2019
@dakahn
Copy link
Contributor

dakahn commented May 14, 2019

Thanks for submitting! The issues is in our backlog, but for an expedited resolution consider contributing a PR to the project.

@jharvey10
Copy link
Contributor

jharvey10 commented Jan 13, 2020

I can reproduce this issue consistently on the storybook by dragging the slider and releasing it while I'm still moving my mouse to the side. If I stop moving for a moment and then release my mouse, typically the onRelease event will fire, but if the mouse is still moving, it almost never fires.

Also, this is much more noticeable on mobile because the touch point of my finger tends to move a bit as I adjust the pressure I'm using to hold/release the slider.

@joshblack joshblack removed the version: 9 Issues pertaining to legacy Carbon label Jan 15, 2020
@joshblack
Copy link
Contributor

Removing the v9 label as this also applies to latest!

joshblack added a commit that referenced this issue Mar 3, 2020
* fix(Slider): onRelease not always firing (#2695)

* test(windows): fixing failing unit tests

* Updating jest `testMatch` micromatch expressions (jestjs/jest#7914)
* Replacing `/` with `path.sep` in icon-build-helpers search.js
* Replacing regex using `/` with `[\\/]` in test-utils scss.js

* refactor(Slider): reworking event logic

- Adding lodash.throttle as project dependency for use in Slider
- Reworking Slider component's event handling
- Including throttling in Slider event handling
- Adjusting CSS for input element on Slider

* test(Slider): updating/adding unit tests

- Increasing (line) test coverage to 100%
- Slight refactor of functions to be more testable
- Correctly handling hidden text input
- Correctly handling touchmove events
- Gracefully handling edge case of zero-width bounding rect

* fix(Slider): code review updates

- Removing usage of functions that aren't available in all browsers
- Adding unit test for display:none style on hidden text input

* fix(Slider): code review updates

- Removing throttling from keydown handling
- Updating unit tests accordingly

* refactor(Slider): use matches module

Using `matches(...)` for arrow key event matching instead of custom impl

Co-authored-by: Josh Black <[email protected]>
Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: Abbey Hart <[email protected]>
joshblack added a commit to joshblack/carbon that referenced this issue Mar 10, 2020
…arbon-design-system#5359)

* fix(Slider): onRelease not always firing (carbon-design-system#2695)

* test(windows): fixing failing unit tests

* Updating jest `testMatch` micromatch expressions (jestjs/jest#7914)
* Replacing `/` with `path.sep` in icon-build-helpers search.js
* Replacing regex using `/` with `[\\/]` in test-utils scss.js

* refactor(Slider): reworking event logic

- Adding lodash.throttle as project dependency for use in Slider
- Reworking Slider component's event handling
- Including throttling in Slider event handling
- Adjusting CSS for input element on Slider

* test(Slider): updating/adding unit tests

- Increasing (line) test coverage to 100%
- Slight refactor of functions to be more testable
- Correctly handling hidden text input
- Correctly handling touchmove events
- Gracefully handling edge case of zero-width bounding rect

* fix(Slider): code review updates

- Removing usage of functions that aren't available in all browsers
- Adding unit test for display:none style on hidden text input

* fix(Slider): code review updates

- Removing throttling from keydown handling
- Updating unit tests accordingly

* refactor(Slider): use matches module

Using `matches(...)` for arrow key event matching instead of custom impl

Co-authored-by: Josh Black <[email protected]>
Co-authored-by: TJ Egan <[email protected]>
Co-authored-by: Abbey Hart <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants