ColorArea and ColorWheel step and pagestep size fit evenly in range #2888

snowystinger · 2022-02-25T01:59:29Z

Closes
Based on changes in #2570
ColorArea behavior is, step and pagestep size fits evenly into the range for RGB with meaningful values.
step is the smallest increment possible, 1
page step steps through web safe colors, 0x11

ColorWheel behavior is, step and pagestep size fits evenly into the hue range for HSL/HSB
step is the smallest increment possible, 1
page step increments by 15, which is large enough to cover the circle quickly but small enough to easily make hitting in between values also short

neither of these are exposed as part of the color area API, instead, when we allow bring your own colorspace, you can set the step and page size through that color space

✅ 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-25T02:04:45Z

Build successful! 🎉

adobe-bot · 2022-02-28T18:46:22Z

Build successful! 🎉

LFDanLu

Confirmed that the thumb snaps to max/min value regardless of the amount of remaining distance to traverse

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

adobe-bot · 2022-02-28T21:31:27Z

Build successful! 🎉

adobe-bot · 2022-02-28T21:35:34Z

Build successful! 🎉

devongovett · 2022-02-28T21:42:41Z

packages/@react-types/color/src/index.d.ts

+  /** The page step size for the xChannel. */
+  xChannelPageStep?: number,
+  /** The page step size for the yChannel. */
+  yChannelPageStep?: number


What's the use case to configure these?

If someone sets a step size bigger than the page step size, then they may want to increase the page step size to compensate.
It might make more sense with HSL/HSB where you can have decimals and you actually want to decrease the page step size.

what if we just multiply the step by some amount to find the page step?

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

Co-authored-by: Michael Jordan <[email protected]>

adobe-bot · 2022-03-02T00:06:39Z

Build successful! 🎉

adobe-bot · 2022-03-02T00:18:40Z

Build successful! 🎉

majornista · 2022-03-02T15:44:05Z

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

    },
    decrementY(stepSize) {
-      setYValue(snapValueToStep(yValue - stepSize, minValueY, maxValueY, stepSize));
+      setYValue(snapValueToStep(yValue - stepSize, minValueY, maxValueY, yChannelStep));
    },


I still think increment/decrement should snap to stepSize rather than xChannelStep or yChannelStep. So that snapping is based on the interval by which one increments or decrements.

For example step=5 pageStep=8

step start at 0 => 5

would it be
pageStep start at 0 => 8 & start at 5 (from the regular step) => 13
or is it
pageStep start at 0 => 8 & start at 5 (from the regular step) => 16

Native sliders, when supplied a step, cannot have a value that does not fall on a multiple of step. You can see this clearly in the Chrome Date picker. I think this is more predictable.
pageStep start at 0 => 8 & start at 5 (from the regular step) => 15

Or are you suggesting that color sliders are different from regular sliders?

Caught up in the other discussion on the same topic. Devon had an idea, we keep the page step size out of the API and just treat it as a multiple of the step. We currently don't support page step size in our Sliders, so this makes a fair amount of sense

I can see where you're coming from if the uses a step size that is not a factor of the page size. With ColorSliders, the step is usually 1, or 0.01 for alpha, and default for pageSize will always snap to step. Can do something like this, using modulus?

incrementX(stepSize) { setXValue(xValue + stepSize > maxValueX ? maxValueX : snapValueToStep(xValue + stepSize, minValueX, maxValueX, stepSize > xChannelStep && stepSize % xChannelStep === 0 ? stepSize : xChannelStep)); }, incrementY(stepSize) { setYValue(yValue + stepSize > maxValueY ? maxValueY : snapValueToStep(yValue + stepSize, minValueY, maxValueY, stepSize > yChannelStep && stepSize % yChannelStep === 0 ? stepSize : yChannelStep)); }, decrementX(stepSize) { setXValue(snapValueToStep(xValue - stepSize, minValueX, maxValueX, stepSize > xChannelStep && stepSize % xChannelStep === 0 ? stepSize : xChannelStep)); }, decrementY(stepSize) { setYValue(snapValueToStep(yValue - stepSize, minValueY, maxValueY, stepSize > yChannelStep && stepSize % yChannelStep === 0 ? stepSize : yChannelStep)); },

it's getting too complicated I think, see previous comment about doing away with it in our API
That way we can make sure it's a multiple of the step and eliminate that worry.

I guess my question would be, what multiple of a step would make the most sense for color channels? The reason for returning pageSize as part of the color channel range object returned by Color.getChannelRange was so that we could have more useful page stepping than simply dividing the range by 10. For example for the hue channel, a pageSize of 15 seems to make sense for snapping to primary, secondary and tertiary colors around the color wheel (BTW, we should use getChannelRange('hue').pageSize instead of the PAGE_MIN_STEP_SIZE constant in useColorWheel). I suggested 0x11 as a pageSize for RGB channels because it divides 0xFF evenly, and increments channel values in a way that makes sense for web developers 0x11, 0x22, 0x33, 0x44, ..., 0xDD, 0xEE, 0xFF.

Another option would be to eliminate step and pageStep from the public API for color controls, and use values that make sense for each color channel internally. Then we wouldn't have to worry about a pageSize that is not a multiple of a step because we would control it.

Maybe this brings up if step should be configurable too?

What is the actual use case for setting step sizes in a color area or color slider? Maybe we should consider that color sliders are different than sliders

RGB 255 is not many values. Having the step size always be 1 and the page size always be 11 makes a lot of sense, you can get quickly to any value and you are always evenly and predictably within the range.

For HSL 0-360 and 0-1 are known ranges that will not change, a step of 1 and 0.01 respectively make sense and page sizes are easy to figure out from there as well.

I think what I'm saying is every color space is well defined and knowable.

I agree that color spaces seem well defined and knowable. I suppose there may be some use cases for floating point steps with hue, saturation and lightness/brightness channels, but it should be noted that Photoshop throws up a dialog if you try to use a floating point value in one of its inputs.

I still think that snapping to stepSize, the pageStep, is preferable to snapping to step, the channel step, and now that we can be sure that the channel step is a factor of the stepSize, I don't se why we shouldn't do it this way.

adobe-bot · 2022-03-03T19:30:02Z

Build successful! 🎉

adobe-bot · 2022-03-03T20:10:49Z

Build successful! 🎉

LFDanLu

Reminder that the aria/spectrum docs for ColorWheel/useColorWheel should also have references/examples to step removed (probably do that in a separate PR, I can help w/ that). The functionality seem good to me

snowystinger · 2022-03-03T21:00:38Z

Yeah, it's in #2666

LFDanLu · 2022-03-03T21:15:39Z

Ok, sweet. I'll go ahead and pull those docs changes out of the PR since those should go in after release

ktabors

Can you add testing instructions? It didn't seem like you mentioned colorslider, but it looks like it changed too.

I tested and keyboard navigation seems good for colorslider. Home and end don't seem to matter for colorwheel since they both goto the same spot.

For color area RTL, home and end key events are still ltr.

snowystinger · 2022-03-03T23:35:27Z

I'm not sure what slider changes you are referring to? This is only ColorArea and ColorWheel.

Home/End are reversed in RTL/LTR ColorArea. That wasn't changed. We can followup if that's weird and it should remain the same.

Correct for ColorWheel, Home/End both go to the same spot. We can follow up if that's weird.

adobe-bot · 2022-03-03T23:48:07Z

Build successful! 🎉

resolved

snowystinger added 2 commits February 24, 2022 18:52

change default step size to evenly fit

c37706f

Bring in max value snapping

338ca6b

snowystinger requested review from dannify and devongovett as code owners February 25, 2022 01:59

snowystinger changed the title ~~change default step size to evenly fit~~ ColorArea step and pagestep size fit evenly in range Feb 25, 2022

Merge branch 'main' into color-area-step-changes

fd6a510

LFDanLu requested changes Feb 28, 2022

View reviewed changes

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

snowystinger and others added 2 commits February 28, 2022 14:25

Allow props to set page step size

5050feb

Merge branch 'main' into color-area-step-changes

fd649ab

devongovett reviewed Feb 28, 2022

View reviewed changes

majornista previously requested changes Feb 28, 2022

View reviewed changes

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

snowystinger and others added 2 commits March 1, 2022 14:26

Update packages/@react-stately/color/src/Color.ts

1860586

Co-authored-by: Michael Jordan <[email protected]>

adjust snap to values and fix tests

4af3db2

Merge branch 'main' into color-area-step-changes

34bf7de

majornista reviewed Mar 2, 2022

View reviewed changes

majornista mentioned this pull request Mar 2, 2022

Color area hsl/hsb #2887

Merged

5 tasks

snowystinger changed the title ~~ColorArea step and pagestep size fit evenly in range~~ ColorArea and ColorWheel step and pagestep size fit evenly in range Mar 3, 2022

snowystinger added 2 commits March 3, 2022 11:16

Remove step and page step from color component APIs

46df036

fix last test

7516b99

Merge branch 'main' into color-area-step-changes

72af055

LFDanLu approved these changes Mar 3, 2022

View reviewed changes

ktabors requested changes Mar 3, 2022

View reviewed changes

ktabors approved these changes Mar 3, 2022

View reviewed changes

Merge branch 'main' into color-area-step-changes

3d4b69c

snowystinger merged commit 1c7aa38 into main Mar 3, 2022

snowystinger deleted the color-area-step-changes branch March 3, 2022 23:51

ktabors mentioned this pull request Mar 4, 2022

Changing locale on ColorArea doesn't change direction thumb moves with home/end key #2921

Closed

snowystinger added the needs testing label Mar 4, 2022

dannify removed the needs testing label Mar 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ColorArea and ColorWheel step and pagestep size fit evenly in range #2888

ColorArea and ColorWheel step and pagestep size fit evenly in range #2888

snowystinger commented Feb 25, 2022 •

edited

Loading

adobe-bot commented Feb 25, 2022

adobe-bot commented Feb 28, 2022

LFDanLu left a comment

adobe-bot commented Feb 28, 2022

adobe-bot commented Feb 28, 2022

devongovett Feb 28, 2022

snowystinger Feb 28, 2022

devongovett Mar 2, 2022

adobe-bot commented Mar 2, 2022

adobe-bot commented Mar 2, 2022

majornista Mar 2, 2022

snowystinger Mar 2, 2022

snowystinger Mar 2, 2022

majornista Mar 2, 2022

snowystinger Mar 2, 2022

majornista Mar 2, 2022

majornista Mar 2, 2022

snowystinger Mar 2, 2022 •

edited

Loading

majornista Mar 3, 2022

majornista Mar 3, 2022

adobe-bot commented Mar 3, 2022

adobe-bot commented Mar 3, 2022

LFDanLu left a comment

snowystinger commented Mar 3, 2022

LFDanLu commented Mar 3, 2022

ktabors left a comment

snowystinger commented Mar 3, 2022 •

edited

Loading

adobe-bot commented Mar 3, 2022

ColorArea and ColorWheel step and pagestep size fit evenly in range #2888

ColorArea and ColorWheel step and pagestep size fit evenly in range #2888

Conversation

snowystinger commented Feb 25, 2022 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

adobe-bot commented Feb 25, 2022

adobe-bot commented Feb 28, 2022

LFDanLu left a comment

Choose a reason for hiding this comment

adobe-bot commented Feb 28, 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

adobe-bot commented Mar 2, 2022

adobe-bot commented Mar 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowystinger Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adobe-bot commented Mar 3, 2022

adobe-bot commented Mar 3, 2022

LFDanLu left a comment

Choose a reason for hiding this comment

snowystinger commented Mar 3, 2022

LFDanLu commented Mar 3, 2022

ktabors left a comment

Choose a reason for hiding this comment

snowystinger commented Mar 3, 2022 • edited Loading

adobe-bot commented Mar 3, 2022

snowystinger commented Feb 25, 2022 •

edited

Loading

snowystinger Mar 2, 2022 •

edited

Loading

snowystinger commented Mar 3, 2022 •

edited

Loading