Slider component for @react-stately and @react-aria #809

chungwu · 2020-07-21T23:33:02Z

This creates:

useMultiSliderState for @react-stately/slider for managing slider state for multiple values
useMultiSlider for @react-aria/slider for managing interactions and accessibility for slider components

Includes some Storybook stories and toy components using the above hooks to build sliders.

Does not include slider implementations for @react-aria/spectrum.

Closes #763

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

For now, just visual tests on Storybook under the "Slider" group, starting here: http://localhost:9003/?path=/story/slider--single

🧢 Your Project:

Plasmic

devongovett · 2020-07-22T04:09:23Z

Skimmed this just now and it looks awesome! Will take a more in depth look tomorrow. 😍

devongovett

Looks really great so far! Will discuss a couple API things with the team tomorrow, but here's a few suggestions/comments to start with. 😄

packages/@react-aria/slider/src/useMultiSlider.ts

packages/@react-aria/slider/src/useSliderThumb.ts

packages/@react-types/slider/README.md

packages/@react-stately/slider/src/useMultiSliderState.ts

devongovett · 2020-07-23T04:38:47Z

A couple other high level things:

Should handles be able to overlap? Noticed that you can drag them directly on top of one another. Should we limit dragging so it can only come within some minimum distance (or touching)?
We'll need to adjust things so it works on touch devices. At the moment the handles don't move. I imagine this has more to do with the existing useDrag1D hook than anything you've implemented, so totally not your fault. And we can get this PR in as an initial implementation and follow up with touch support later as well. I think eventually we'll want to move that hook into @react-aria/interactions and add the touch support there. This will also apply to the mouse events that you've added in slider eventually as well.

Thanks again for this awesome (and quick) work! 😍

chungwu · 2020-07-24T05:49:45Z

@devongovett Many thanks for the quick response! Makes sense to me; will get back to this soon 😅

Re: overlapping handles -- this seems like an opinionated decision that perhaps should be made by the component author (so, for example, would live in react-spectrum and not react-aria)? react-aria will give you the offset percentages of each handle, and it is up to you to decide how you want to render those handles (like doing a slight translateX if two handles have the same offset)...

devongovett · 2020-07-25T00:15:35Z

That makes sense to me!

chungwu · 2020-07-28T02:10:44Z

I've taken a pass at addressing the feedback. Once there's consensus on the API and the behavior, I can start writing up some tests 😄

devongovett

Looks great. A couple of small API things. Other than that, looks good to move forward on tests when you have time! 😄

packages/@react-aria/slider/src/useSlider.ts

devongovett · 2020-07-29T02:08:13Z

packages/@react-aria/slider/src/useSliderThumb.ts

+    onIncrement: () => state.setValue(index, value + step),
+    onDecrement: () => state.setValue(index, value - step),
+    onIncrementToMax: () => state.setValue(index, maxValue),
+    onDecrementToMin: () => state.setValue(index, minValue)


I don't think these are working right now? Home and End aren't doing anything for me. I think you may need to pass through the onKeyDown handler to the input element?

Maybe we could add methods for incrementing/decrementing a thumb to stately? Then you wouldn't need to pass through minValue and maxValue as well. These are somewhat confusing since they are the minimum/maximum of the whole slider rather than the individual thumb. Setting the value only works because it is clamped in state.setValue. So maybe having a method like state.incrementToMax(index) would be clearer?

Hmm, weird; Home and End do work for me (Mac/Chrome, Mac/Safari, Linux/Chrome). The focus is on the input[type=range] control, so as long as the browser handles those shortcuts, we should see the changes in the onChange handler on the input... 🤔

We actually don't need to pass in onIncrement/onIncrementMax/etc. at all now, since we're just letting the browser handle the stepping... Will remove!

(Could you let me know the browser/OS combo where you're seeing the Home/End issue?)

It was macOS safari. But let me check again.

packages/@react-aria/slider/src/useSliderThumb.ts

packages/@react-stately/slider/src/useSliderState.ts

chungwu · 2020-08-03T05:09:52Z

Hi all! Just took another pass addressing the feedback, as well as adding tests for useSliderState, useSlider, and useSliderThumb.

The only piece of feedback I'm still hesitating about is this: #809 (comment) Again, happy to do what you believe is most conventional here, just wanted to point out a few things!

(else once focus is on a draggable, you cannot tab out of it)

Example components built from them are in @react-aria/slider/stories, with some Storybook stories as well. No unit tests yet.

Instead of handling keyboard interactions ourselves via useDrag1D's onKeyDown handler

chungwu · 2020-08-05T05:07:33Z

I believe I've resolved all the remaining comments (except for keyboard shortcuts not working for @devongovett when he tried it last). Thanks for the feedback!

devongovett · 2020-08-05T05:17:04Z

Excellent! Will take one final look tomorrow morning and get it merged. 😄

devongovett

Nice work! Thanks again. 😄

chungwu · 2020-08-05T20:39:42Z

Yay! It's clear you folks really prioritize community involvement, and working on this PR was an awesome experience 😍

chungwu requested review from dannify, devongovett and snowystinger as code owners July 21, 2020 23:33

chungwu mentioned this pull request Jul 21, 2020

useSlider() and useSliderState() for a Slider component #763

Closed

devongovett reviewed Jul 23, 2020

View reviewed changes

chungwu force-pushed the main branch from a62422d to 6f7ff0c Compare July 28, 2020 02:09

chungwu force-pushed the main branch from 6f7ff0c to 7f7bce6 Compare July 28, 2020 19:14

devongovett reviewed Jul 29, 2020

View reviewed changes

chungwu force-pushed the main branch from 7f7bce6 to 2acdd65 Compare August 3, 2020 05:03

chungwu added 11 commits August 4, 2020 21:56

useDrag1D should only trap keyboard events if handled

4493489

(else once focus is on a draggable, you cannot tab out of it)

First stab at @react-aria/slider and @react-stately/slider

21f44f6

Example components built from them are in @react-aria/slider/stories, with some Storybook stories as well. No unit tests yet.

Rename MultiSlider to just Slider

6591289

useSlider() returns commonThumbProps(), to be passed to useSliderThumb()

1698447

Set thumb value immediately onMouseDown of the track

270f059

Turn parameters to useSliderThumb into options object

09dea5d

Rely on range inputs to handle keyboard interactions

3cd7a7e

Instead of handling keyboard interactions ourselves via useDrag1D's onKeyDown handler

Make sure realTimeDragging is updated if multiple thumbs are dragged

6e0e1f0

Fix some lint issues

f0e0f74

Implement feedback renaming a few things

51ce54c

Add tests for useSliderState, useSlider, useSliderThumb

e34e9c5

chungwu force-pushed the main branch 3 times, most recently from 5881e60 to a075c63 Compare August 5, 2020 05:05

Have SliderThumbOpts extend SliderThumbProps

258896f

chungwu force-pushed the main branch from a075c63 to 258896f Compare August 5, 2020 05:06

devongovett approved these changes Aug 5, 2020

View reviewed changes

devongovett merged commit 591c284 into adobe:main Aug 5, 2020

itaylor mentioned this pull request Sep 10, 2020

useDrag1D should use pointer events instead of (or in addition to) mouse events #1063

Closed

Slider component for @react-stately and @react-aria #809

Slider component for @react-stately and @react-aria #809

Uh oh!

Conversation

chungwu commented Jul 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

Uh oh!

devongovett commented Jul 22, 2020

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

devongovett commented Jul 23, 2020

Uh oh!

chungwu commented Jul 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devongovett commented Jul 25, 2020

Uh oh!

chungwu commented Jul 28, 2020

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

devongovett Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

devongovett Jul 29, 2020

Choose a reason for hiding this comment

Uh oh!

chungwu Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

chungwu Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

devongovett Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chungwu commented Aug 3, 2020

Uh oh!

chungwu commented Aug 5, 2020

Uh oh!

devongovett commented Aug 5, 2020

Uh oh!

devongovett left a comment

Choose a reason for hiding this comment

Uh oh!

chungwu commented Aug 5, 2020

Uh oh!

Uh oh!

chungwu commented Jul 21, 2020 •

edited

Loading

chungwu commented Jul 24, 2020 •

edited

Loading