fix(#2664): useColorSlider: refine PageUp/PageDown behaviors #2666

majornista · 2021-12-09T02:37:31Z

Refine snapValueToStep for incrementing or decrementing from max.
Remove step/pageSize from ColorSlider and ColorField API
Fixes same thing this PR and its predecessor addressed Fix useNumberFieldStately dep on aria #2918, the flicker of the previous value before showing the new value

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue 2664.
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:

PageUp/Down should work for ColorSlider and should increment and decrement the value by the pageStep size. pageStep size is determined by the color channel.
For rgb channels, pageStep is 17
For alpha/satuaration/other percentage channesl, pageStep is .1
For hue, pageStep is 15 degrees

values reached via pageUp/Down should always be values reachable via the step

🧢 Your Project:

Adobe/Accessibility

adobe-bot · 2021-12-09T03:05:24Z

Build successful! 🎉

adobe-bot · 2021-12-09T16:39:25Z

Build successful! 🎉

reidbarber

LGTM, tested in storybook. Also, link in the test instructions should probably be updated to go the ColorArea story

LFDanLu

Feels kinda weird that onChangeEnd happens when pressing Left/Right/Up/Down at either extremes but PageUp/Down/End/Home don't but I'm fine with addressing that in a separate PR

EDIT: Ah never mind, I see that you've fixed the above behavior in #2661

packages/@react-spectrum/color/stories/ColorArea.stories.tsx

packages/@react-spectrum/color/test/ColorSlider.test.tsx

adobe-bot · 2022-01-26T22:18:14Z

Build successful! 🎉

LFDanLu

LGTM

LFDanLu · 2022-01-28T00:44:33Z

cc @snowystinger if you wanna take a quick look since this is pointed towards your color area branch

adobe-bot · 2022-02-07T18:56:48Z

Build successful! 🎉

adobe-bot · 2022-02-18T15:14:57Z

Build successful! 🎉

adobe-bot · 2022-02-24T01:39:05Z

Build successful! 🎉

adobe-bot · 2022-02-28T15:20:42Z

Build successful! 🎉

snowystinger · 2022-02-28T17:33:55Z

packages/@react-aria/color/src/useColorSlider.ts

+        state.setValue(state.value.withChannelValue(channel, newValue));
+      }
+      // wait a frame to ensure value has changed then unset this so that onChangeEnd is fired
+      requestAnimationFrame(() => state.setThumbDragging(0, false));


we should be able to structure this the same way as useSlider https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/slider/src/useSliderThumb.ts#L115
so that we don't need the raf

#2570 (comment) might be a concern, lemme double check

snowystinger · 2022-02-28T17:37:39Z

packages/@react-aria/color/src/useColorSlider.ts

+      let newValue = channelValue;
+      switch (e.key) {
+        case 'PageUp':
+          newValue = channelValue + pageStep > maxValue ? maxValue : snapValueToStep(channelValue + pageStep, minValue, maxValue, pageStep);


useColorSliderState should have increment/decrement thumb functions https://github.com/adobe/react-spectrum/blob/main/packages/%40react-stately/color/src/useColorSliderState.ts
see
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-stately/slider/src/useSliderState.ts#L125

We should try to match this logic to useSlider as best we can

Just a heads up, but I think @majornista was running into some issues w/ using the setThumbValue not updating the color value here, need to double check myself

interesting, I reopened that PR with a pared-down set of changes, #2887
it's passing the tests, I think I need more info on what was going wrong

adobe-bot · 2022-02-28T22:55:10Z

Build successful! 🎉

- Omit change events when value doesn't change. - Refine snapValueToStep for incrementing or decrementing from max.

adobe-bot · 2022-02-28T23:06:53Z

Build successful! 🎉

packages/@react-stately/color/src/useColorSliderState.ts

…Size If the channel range step is less than 1, like 0.01, and stepSize is not passed as a prop, the slider will always increment or decrement by 1.

adobe-bot · 2022-03-01T15:58:47Z

Build successful! 🎉

adobe-bot · 2022-03-04T00:25:51Z

Build successful! 🎉

adobe-bot · 2022-03-04T00:25:54Z

Build successful! 🎉

adobe-bot · 2022-03-04T02:15:56Z

Build successful! 🎉

devongovett · 2022-03-04T16:13:45Z

packages/@react-aria/color/src/useColorSlider.ts

+      // wait a frame to ensure value has changed then unset this so that onChangeEnd is fired
+      state.setThumbDragging(0, false);
+    }
+  });


Doesn't useSliderThumb handle this? I saw the above discussion but I'm not sure why it is resolved?

I had readded it cuz the behavior broke with the removal of pageSize from useSliderState props in #2917, but I can re-remove it since we want to keep pageSize prop in useSliderState

EDIT: re-removed since we now overwrite useSliderState's step/pageSize with the color channel's specific step/pageSize in useColorSliderState

reidbarber

LGTM

adobe-bot · 2022-03-04T17:56:18Z

Build successful! 🎉

LFDanLu · 2022-03-04T19:11:34Z

packages/@react-stately/color/src/useColorSliderState.ts

+  function incrementThumb(index: number, stepSize: number = 1) {
+    let {maxValue, minValue, step} = color.getChannelRange(channel);
+    let channelValue = color.getChannelValue(channel);
+    let s = Math.max(stepSize, step);
+    sliderState.setThumbValue(index, channelValue + s > maxValue ? maxValue : snapValueToStep(channelValue + s, minValue, maxValue, s));
+  }
+
+  function decrementThumb(index: number, stepSize: number = 1) {
+    let {maxValue, minValue, step} = color.getChannelRange(channel);
+    let channelValue = color.getChannelValue(channel);
+    let s = Math.max(stepSize, step);
+    sliderState.setThumbValue(index, snapValueToStep(channelValue - s, minValue, maxValue, s));
+  }


Do we actually need these increment/decrementThumb functions? Think we don't need them now that ColorSliderState exposes the correct step/pageStep size?

Did some testing, I think we don't need these, I believe useSliderState's increment/decrementThumb + useSliderThumb is now getting the correct step/pageStep size with the changes to ColorSliderState. I'll go ahead and push the changes, note I needed to modify

react-spectrum/packages/@react-spectrum/color/test/ColorSlider.test.tsx

Lines 304 to 306 in 03d5ebe

expect(onChangeSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));

expect(onChangeEndSpy).toHaveBeenCalledTimes(5);

expect(onChangeEndSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));

, those should be expecting 1 rather than 0 since the step is 17 for PageUp/Down

Ah wait, nevermind, we want this snapping behavior onPageUp/Down.

EDIT: actually do we want this snapping behavior for onPageUp/Down? Original issue doesn't describe the desired behavior, ColorArea doesn't work like this but ColorWheel does?
eg. Red slider, current value is 2, pageStep is 17 as per channelValue pageStep. Press pageUp -> value snaps to 17 rather than 19?
cc @majornista @snowystinger @devongovett

if a step is provided, we snap to the step, is that what you're asking?

What about if a pageSize is provided? Should pageUp/Down always snap to a pageSize value or should it just increment/decrement by the pageSize value? Right now in this PR, ColorSlider will snap to a pageStep value upon using pageUp/Down (see the example I listed about) but this diverges from the behavior on main where pageUp/Down will increment by the pageSize value.

If it shouldn't snap to the pageStep value then I'll go ahead and remove the increment/decrementThumb functions but if that behavior is correct, then should we update the ColorArea to behave the same way

a step cannot be supplied to a colorslider, nor can a page size, those are determined in the color space

pageSize should always snap to the step

…/react-spectrum into Issue-2664-useColorSlider-pageSize

adobe-bot · 2022-03-04T20:23:21Z

Build successful! 🎉

adobe-bot · 2022-03-04T20:28:58Z

Build successful! 🎉

addressed

adobe-bot · 2022-03-04T20:57:33Z

Build successful! 🎉

adobe-bot · 2022-03-04T21:16:47Z

Build successful! 🎉

adobe-bot · 2022-03-04T21:25:03Z

Build successful! 🎉

adobe-bot · 2022-03-04T21:28:45Z

Build successful! 🎉

majornista requested review from dannify, devongovett and snowystinger as code owners December 9, 2021 02:37

majornista force-pushed the Issue-2664-useColorSlider-pageSize branch from 61a13d3 to 8e9b59b Compare December 9, 2021 02:55

majornista force-pushed the Issue-2664-useColorSlider-pageSize branch from 8e9b59b to 8573563 Compare December 9, 2021 16:30

LFDanLu linked an issue Dec 15, 2021 that may be closed by this pull request

useColorSlider should increment/decrement by channel pageSize using PageUp/PageDown #2664

Closed

reidbarber previously approved these changes Dec 21, 2021

View reviewed changes

LFDanLu requested changes Jan 25, 2022

View reviewed changes

packages/@react-spectrum/color/stories/ColorArea.stories.tsx Outdated Show resolved Hide resolved

packages/@react-spectrum/color/test/ColorSlider.test.tsx Show resolved Hide resolved

LFDanLu previously approved these changes Jan 28, 2022

View reviewed changes

majornista force-pushed the Issue-2664-useColorSlider-pageSize branch from d2d0fc7 to 8256492 Compare February 24, 2022 01:33

Base automatically changed from color-area to main February 25, 2022 00:07

snowystinger dismissed stale reviews from LFDanLu and reidbarber via 8256492 February 25, 2022 00:07

snowystinger reviewed Feb 28, 2022

View reviewed changes

majornista added 2 commits February 28, 2022 17:56

fix(#2664): useColorSlider: refine PageUp/PageDown behaviors

463837d

- Omit change events when value doesn't change. - Refine snapValueToStep for incrementing or decrementing from max.

fix(#2664): useColorSlider: simplify per code review

f533056

majornista force-pushed the Issue-2664-useColorSlider-pageSize branch from c61bbb6 to f533056 Compare February 28, 2022 23:01

snowystinger reviewed Feb 28, 2022

View reviewed changes

packages/@react-stately/color/src/useColorSliderState.ts Outdated Show resolved Hide resolved

fix(#2664): useColorSlider: fix increment/decrementThumb default step…

60606e3

…Size If the channel range step is less than 1, like 0.01, and stepSize is not passed as a prop, the slider will always increment or decrement by 1.

devongovett reviewed Mar 4, 2022

View reviewed changes

reidbarber previously approved these changes Mar 4, 2022

View reviewed changes

LFDanLu dismissed reidbarber’s stale review via bebd2cc March 4, 2022 17:39

LFDanLu force-pushed the Issue-2664-useColorSlider-pageSize branch from 62c7693 to bebd2cc Compare March 4, 2022 17:39

Merge branch 'main' into Issue-2664-useColorSlider-pageSize

03d5ebe

LFDanLu reviewed Mar 4, 2022

View reviewed changes

LFDanLu and others added 3 commits March 4, 2022 11:49

removing extraneous increment/decrementThumb from useColorSliderState

ef0291d

move to refs

42cb9f0

Merge branch 'Issue-2664-useColorSlider-pageSize' of github.com:adobe…

8f6a1ca

…/react-spectrum into Issue-2664-useColorSlider-pageSize

snowystinger approved these changes Mar 4, 2022

View reviewed changes

devongovett approved these changes Mar 4, 2022

View reviewed changes

Merge branch 'main' into Issue-2664-useColorSlider-pageSize

86cf7ce

Merge branch 'main' into Issue-2664-useColorSlider-pageSize

b66d897

Merge branch 'main' into Issue-2664-useColorSlider-pageSize

6e42b4a

LFDanLu added the needs testing label Mar 4, 2022

Merge branch 'main' into Issue-2664-useColorSlider-pageSize

cf196db

snowystinger merged commit 3c89231 into main Mar 4, 2022

snowystinger deleted the Issue-2664-useColorSlider-pageSize branch March 4, 2022 21:31

dannify removed the needs testing label Mar 8, 2022

	expect(onChangeSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));
	expect(onChangeEndSpy).toHaveBeenCalledTimes(5);
	expect(onChangeEndSpy.mock.calls[4][0].toString('hexa')).toBe(defaultColor.withChannelValue('red', 0).toString('hexa'));

fix(#2664): useColorSlider: refine PageUp/PageDown behaviors #2666

fix(#2664): useColorSlider: refine PageUp/PageDown behaviors #2666

Conversation

majornista commented Dec 9, 2021 • edited by LFDanLu Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Dec 9, 2021

adobe-bot commented Dec 9, 2021

reidbarber left a comment

Choose a reason for hiding this comment

LFDanLu left a comment • edited Loading

Choose a reason for hiding this comment

adobe-bot commented Jan 26, 2022

LFDanLu left a comment

Choose a reason for hiding this comment

LFDanLu commented Jan 28, 2022

adobe-bot commented Feb 7, 2022

adobe-bot commented Feb 18, 2022

adobe-bot commented Feb 24, 2022

adobe-bot commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Feb 28, 2022

adobe-bot commented Feb 28, 2022

adobe-bot commented Mar 1, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

Choose a reason for hiding this comment

LFDanLu Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

reidbarber left a comment

Choose a reason for hiding this comment

adobe-bot commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LFDanLu Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowystinger Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

adobe-bot commented Mar 4, 2022

majornista commented Dec 9, 2021 •

edited by LFDanLu

Loading

LFDanLu left a comment •

edited

Loading

LFDanLu Mar 4, 2022 •

edited

Loading

LFDanLu Mar 4, 2022 •

edited

Loading

snowystinger Mar 4, 2022 •

edited

Loading