Have slider work like color area #2819

snowystinger · 2022-02-08T01:04:37Z

Closes

This matches our work in color area and color slider.
Yes, we could rely on native behavior, but it causes the code to be fairly different and would make it harder in the future to support custom page size steps.
It also allows us to run some tests we couldn't run before.

✅ 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:

🧢 Your Project:

adobe-bot · 2022-02-08T01:10:12Z

Build successful! 🎉

majornista · 2022-02-08T01:21:07Z

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

@@ -77,44 +77,79 @@ export function useSliderThumb(
  stateRef.current = state;
  let reverseX = direction === 'rtl';
  let currentPosition = useRef<number>(null);
+
+  let pageSize = 1;


Should default page size be 1 or something like (getThumbMaxValue(index) - getThumbMinValue(index)) / 10, so that PageUp/PageDown behaves like a native input[type=range]?

Also, should pageSize be a public API distinct from step? So that things like ColorSlider can increment or decrement RGB values by 0x10 using PageUp/PageDown while step remains 0x1.

Also, should pageSize be a public API distinct from step? So that things like ColorSlider can increment or decrement RGB values by 0x10 using PageUp/PageDown while step remains 0x1.

I think it can be, not sure if we want to open that in this PR just to minimize changes we make. Would it simplify some of our code elsewhere? That would be a reason to do it.

Should default page size be 1 or something like (getThumbMaxValue(index) - getThumbMinValue(index)) / 10, so that PageUp/PageDown behaves like a native input[type=range]?

Interesting, I didn't pick up that that was what native was doing. I think that'd be ok.

(getThumbMaxValue(index) - getThumbMinValue(index)) / 10 might be weird for RangeSlider where the min and max for each thumb value is constrained. I think this is yet another reason for a public API that distinguishes pageSize from step.

adobe-bot · 2022-02-08T18:24:56Z

Build successful! 🎉

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

majornista

Should pageSize default to Math.max((maxValue - minValue) / 10), step)?

adobe-bot · 2022-02-09T19:29:47Z

Build successful! 🎉

ktabors

I don't think we support a vertical slider, but we should probably add a vertical slider story in the hooks section.
Also, a pageSize story.

ktabors · 2022-02-09T19:34:59Z

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

+          } else {
+            stateRef.current.decrementThumb(index, shiftKey ? pageSize : step);
+          }
+        } else if (deltaY > 0) {


Would it make sense to group the deltaX and deltaY portions?

I am not sure I know what you're saying? I think this is pretty easy to follow, only X direction is influenced by locale direction

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

majornista · 2022-02-10T22:53:24Z

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

-        if (delta < 0 && currentValue === getThumbMinValue(index)) {
-          return;
+        if (deltaX > 0) {
+          if (direction === 'rtl') {


We have reverseX declared above at https://github.com/adobe/react-spectrum/pull/2819/files#diff-9eebdd2756162e835cc5a37501ce3ccc3decdfd5d1dee4345d2e5f5a06c38703R78

…direction === 'rtl'

adobe-bot · 2022-02-10T23:04:58Z

Build successful! 🎉

majornista

Do we need storybook examples of pageSize, or should I just add them in Issue-2659-useSliderThumb-onChangeEnd after merging.

…2659 / adobe#2819) adobe#2

@trivial

…/End (#2661) * useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End * fix(#2659): add onChangeEnd event handler to Slider/RangeSlider stories Per #2661 (review) * Have slider work like color area (#2819) * Match Slider implementation to color area * fix lint * make step optional with a default * add pagesize * Use native slider pagesize calculation * @trivial remove irrelevant comment and use the declared variable for direction === 'rtl' Co-authored-by: Michael Jordan <[email protected]> * fix(#2659): add pageSize stories * simplify slider move logic * a little more cleanup Co-authored-by: Daniel Lu <[email protected]> Co-authored-by: Robert Snow <[email protected]> Co-authored-by: Daniel Lu <[email protected]>

…2659 / adobe#2819) adobe#2

@trivial

…/Home/End (adobe#2661) * useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End * fix(adobe#2659): add onChangeEnd event handler to Slider/RangeSlider stories Per adobe#2661 (review) * Have slider work like color area (adobe#2819) * Match Slider implementation to color area * fix lint * make step optional with a default * add pagesize * Use native slider pagesize calculation * @trivial remove irrelevant comment and use the declared variable for direction === 'rtl' Co-authored-by: Michael Jordan <[email protected]> * fix(adobe#2659): add pageSize stories * simplify slider move logic * a little more cleanup Co-authored-by: Daniel Lu <[email protected]> Co-authored-by: Robert Snow <[email protected]> Co-authored-by: Daniel Lu <[email protected]>

snowystinger added 3 commits February 1, 2022 15:16

Match Slider implementation to color area

f270683

fix lint

dbb0d0f

make step optional with a default

45f8553

snowystinger requested review from dannify and devongovett as code owners February 8, 2022 01:04

majornista reviewed Feb 8, 2022

View reviewed changes

add pagesize

16c304d

majornista reviewed Feb 8, 2022

View reviewed changes

packages/@react-stately/slider/src/useSliderState.ts Show resolved Hide resolved

majornista requested changes Feb 8, 2022

View reviewed changes

Use native slider pagesize calculation

433886f

ktabors requested changes Feb 9, 2022

View reviewed changes

majornista reviewed Feb 10, 2022

View reviewed changes

packages/@react-aria/slider/src/useSliderThumb.ts Show resolved Hide resolved

majornista reviewed Feb 10, 2022

View reviewed changes

packages/@react-aria/slider/src/useSliderThumb.ts Outdated Show resolved Hide resolved

majornista reviewed Feb 10, 2022

View reviewed changes

@trivial remove irrelevant comment and use the declared variable for …

6c0281a

…direction === 'rtl'

majornista approved these changes Feb 10, 2022

View reviewed changes

majornista merged commit 92cb6ee into Issue-2659-useSliderThumb-onChangeEnd Feb 11, 2022

majornista deleted the have-slider-work-like-color-area branch February 11, 2022 16:29

majornista added a commit to majornista/react-spectrum-v3 that referenced this pull request Feb 14, 2022

useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End (adobe#…

bd9635e

…2659 / adobe#2819) adobe#2

majornista added a commit to majornista/react-spectrum-v3 that referenced this pull request Feb 18, 2022

useSliderThumb: fire onChangeEnd for PageUp/PageDown/Home/End (adobe#…

6388ff5

…2659 / adobe#2819) adobe#2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Have slider work like color area #2819

Have slider work like color area #2819

snowystinger commented Feb 8, 2022

adobe-bot commented Feb 8, 2022

majornista Feb 8, 2022

majornista Feb 8, 2022

snowystinger Feb 8, 2022

majornista Feb 8, 2022

adobe-bot commented Feb 8, 2022

majornista left a comment

adobe-bot commented Feb 9, 2022

ktabors left a comment

ktabors Feb 9, 2022

snowystinger Feb 9, 2022

majornista Feb 10, 2022

adobe-bot commented Feb 10, 2022

majornista left a comment

Have slider work like color area #2819

Have slider work like color area #2819

Conversation

snowystinger commented Feb 8, 2022

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Feb 8, 2022

majornista Feb 8, 2022

Choose a reason for hiding this comment

majornista Feb 8, 2022

Choose a reason for hiding this comment

snowystinger Feb 8, 2022

Choose a reason for hiding this comment

majornista Feb 8, 2022

Choose a reason for hiding this comment

adobe-bot commented Feb 8, 2022

majornista left a comment

Choose a reason for hiding this comment

adobe-bot commented Feb 9, 2022

ktabors left a comment

Choose a reason for hiding this comment

ktabors Feb 9, 2022

Choose a reason for hiding this comment

snowystinger Feb 9, 2022

Choose a reason for hiding this comment

majornista Feb 10, 2022

Choose a reason for hiding this comment

adobe-bot commented Feb 10, 2022

majornista left a comment

Choose a reason for hiding this comment