ColorArea: add HSB and HSL support #2570

majornista · 2021-11-19T16:23:12Z

Closes #1732 Closes #1118

Opening this PR mainly so I don't lose track of this work.

Storybook examples:

✅ 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/Accessibility

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

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

LFDanLu · 2022-02-23T23:53:34Z

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

+/* TODO: what does a disabled color area look like? */
+export let XHueYSaturationHSLisDisabled = Template.bind({});
+XHueYSaturationHSLisDisabled.storyName = 'HSL xChannel="hue", yChannel="saturation", isDisabled';
+XHueYSaturationHSLisDisabled.args = {...XHueYSaturationHSL.args, isDisabled: true};


Good question, maybe something to bring up to spectrum? I think making everything gray (like what you have) is probably easiest since we won't have to worry about if the disabled handle is too hard too see and it maybe confusing if we only change the handle style but leave the color area filled in

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

…ax value ColorArea: refine pageup/pagedown behaviors Omit change events when value doesn't change. useColorAreaState: fix onChangeEnd only after actual change

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

Demonstrates controlled state and color conversion from HSL/HSB/RGB in ColorArea/ColorWheel/ColorSlider and ColorField, which is currently RGB only.

…nnels

ktabors · 2022-02-24T21:46:53Z

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

-    formatMessage('colorNameAndValue', {name: state.value.getChannelName('green', locale), value: state.value.formatChannelValue('green', locale)}),
-    formatMessage('colorNameAndValue', {name: state.value.getChannelName('blue', locale), value: state.value.formatChannelValue('blue', locale)})
-  ].join(', ');
+  let getValueTitle = () => {


Is this change going to cause issues if I'm using an app and I change my locale? I'm also curious if there could be issues with this component being used in a collaborative app where one person has their locale in one language/region and the other has it set to a different one. This code should be transforming the data for display to the user, but double checking.

ktabors · 2022-02-24T21:53:42Z

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);


Why would pressing PageUp ever cause the user to see the maxValue? Isn't PageUp moving upward, i.e. closer to the minValue? When I tested this pageup/pagedown seemed backwards.

Also, why does snapValueToStep have this broder issue when moving up, but not with PageDown? This feels like a bug with snapValueToStep. I noticed in useColorAreaState.ts there was this min/max check on both values not just one like here.

ktabors · 2022-02-24T21:56:31Z

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

+          break;
+      }
+      // remember to set this so that onChangeEnd is fired
+      state.setThumbDragging(0, true);


Shouldn't the setThumDragging only change if the value changes?

We need to set it and then immediately unset it just so that onChangeEnd is fired

ktabors · 2022-02-24T22:39:06Z

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

+  let isHue = zChannel === 'hue';
+  function onChange(e) {
+    try {
+      e = normalizeColor(e);


Why is normalizeColor throwing an error you need to catch?

Adding ColorField to the story means that parseColor might throw an error if the string entered into the ColorField cannot be parsed to a color.

export function normalizeColor(v: string | IColor) { if (typeof v === 'string') { return parseColor(v); } else { return v; } }

ktabors · 2022-02-24T22:41:01Z

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

+XLightnessYSaturation.storyName = 'HSL xChannel="lightness", yChannel="saturation"';
+XLightnessYSaturation.args = {xChannel: 'lightness', yChannel: 'saturation', defaultValue: 'hsl(0, 100%, 50%)', onChange: action('onChange'), onChangeEnd: action('onChangeEnd')};
+
+/* TODO: what does a disabled color area look like? */


Is this todo resolved or need to another PR or an issue?

I think we've got this pretty much resolved, I'll remove the comment, thanks

ktabors · 2022-02-24T22:41:43Z

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

+
+export let XSaturationYLightness = Template.bind({});
+XSaturationYLightness.storyName = 'HSL xChannel="saturation", yChannel="lightness"';
+XSaturationYLightness.args = {xChannel: 'saturation', yChannel: 'lightness', defaultValue: 'hsl(0, 100%, 50%)', onChange: action('onChange'), onChangeEnd: action('onChangeEnd')};


why do we care about the onChange and onChangeEnd events for these stories? I'm speaking to these not being on every RGB story.

they are on every rgb story in the latest on the color-area branch, they're inherited from the base one

ktabors · 2022-02-24T22:51:44Z

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

@@ -120,3 +147,81 @@ XBlueYGreenSize3000.args = {...XBlueYGreen.args, size: 'size-3000'};
 export let XBlueYGreenSize600 = Template.bind({});
 XBlueYGreenSize600.storyName = 'RGB xChannel="blue", yChannel="green", size="size-600"';
 XBlueYGreenSize600.args = {...XBlueYGreen.args, size: 'size-600'};
+
+export let XSaturationYLightness = Template.bind({});


This a lot of new stories to test. I know we are just following the pattern of the RGB stories above. Wondering if it would make more sense to add these to chromatic storybook and only having one or two examples of this or a dropdown to change a single story between all of these, or checkboxes to change toggle the examples between all these combinations?

ktabors · 2022-02-24T23:00:56Z

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

-        case 'green':
-          xChannel = 'blue';
+        case 'hue':
+          xChannel = colorSpace === 'hsb' ? 'brightness' : 'lightness';


are we 100% sure there will only be three color spaces in react spectrum? only hsb, rgb and hsl? If not I don't like this ternary. Never supporting YUV or CMYK?

this is just for now

majornista requested review from dannify, devongovett and snowystinger as code owners November 19, 2021 16:23

majornista added the enhancement New feature or request label Dec 2, 2021

majornista mentioned this pull request Dec 2, 2021

Color area #2483

Merged

5 tasks

majornista force-pushed the color-area-hsl-hsb branch 4 times, most recently from 43990a0 to 4ce326f Compare December 8, 2021 21:58

majornista mentioned this pull request Dec 8, 2021

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

Closed

majornista force-pushed the color-area-hsl-hsb branch 2 times, most recently from 0e52041 to 8b8ea3a Compare December 9, 2021 17:05

majornista force-pushed the color-area-hsl-hsb branch 4 times, most recently from 27e9c46 to 2856bcf Compare February 18, 2022 19:11

LFDanLu requested changes Feb 23, 2022

View reviewed changes

majornista added 5 commits February 23, 2022 20:12

ColorArea: add HSB and HSL support

b63490a

ColorArea: fix keyboard behavior for rtl locales and snapToStep for m…

ba5698d

…ax value ColorArea: refine pageup/pagedown behaviors Omit change events when value doesn't change. useColorAreaState: fix onChangeEnd only after actual change

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

3ecc458

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

ColorArea: update storybook example to use ColorField

a5466b9

Demonstrates controlled state and color conversion from HSL/HSB/RGB in ColorArea/ColorWheel/ColorSlider and ColorField, which is currently RGB only.

ColorArea: add getChannels method to Color, returning Set of ColorCha…

76922ca

…nnels

majornista force-pushed the color-area-hsl-hsb branch from 2856bcf to 76922ca Compare February 24, 2022 01:19

majornista requested a review from LFDanLu February 24, 2022 01:35

ktabors requested changes Feb 24, 2022

View reviewed changes

snowystinger deleted the branch adobe:color-area February 25, 2022 00:07

snowystinger closed this Feb 25, 2022

This was referenced Feb 25, 2022

Color area hsl/hsb #2887

Merged

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

Merged

LFDanLu mentioned this pull request Feb 28, 2022

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

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ColorArea: add HSB and HSL support #2570

ColorArea: add HSB and HSL support #2570

majornista commented Nov 19, 2021 •

edited

Loading

LFDanLu Feb 23, 2022

ktabors Feb 24, 2022

ktabors Feb 24, 2022

ktabors Feb 24, 2022

snowystinger Feb 24, 2022

ktabors Feb 24, 2022

majornista Mar 2, 2022

ktabors Feb 24, 2022

snowystinger Feb 24, 2022

ktabors Feb 24, 2022

snowystinger Feb 24, 2022

ktabors Feb 24, 2022

ktabors Feb 24, 2022

snowystinger Feb 24, 2022

ColorArea: add HSB and HSL support #2570

ColorArea: add HSB and HSL support #2570

Conversation

majornista commented Nov 19, 2021 • edited Loading

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

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

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

majornista commented Nov 19, 2021 •

edited

Loading