Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ColorPicker: store internal HSLA state for better slider UX #57555

Merged
merged 7 commits into from
Jan 11, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Jan 4, 2024

Fixes #57209

What?

Improve the UX of the ColorPicker when using HSL sliders, so that the use can move each slider freely even when that doesn't result in a change to the final color

Why?

The way sliders work currently represents a bad UX

How?

The issue that we're trying to solve is caused by the fact that, when a slider's value changes, we convert from HSLA to a colord object in the onChange function, and then we convert the received color prop back to HSLA. Converting to a colord object was necessary to have a universal representation of the color, regardless of the color space that the user chooses (RGB, HSL..). But doing so causes the H and S values to get "stuck" on 0 if changing them wouldn't affect the final color value (see detailed explanation in #57209 (comment)).

To get around this limitation, this PR stores the individual H/S/L/A values in local state. When the received color and the internal values produce the same resulting color, we can safely use the local H/S/L/A values (instead of the received color prop), which retain the values that the user selected by dragging the slider.

Testing Instructions

  • Open the ColorPicker storybook example
  • Switch to HSL color space
  • Select pure white and/or pure black as a color. Try interacting with the H and L sliders: the sliders should move when dragging the thumb, and the corresponding NumberControl should also update accordingly. The color should stay unchanged (since, for pure white and black, changing H and S values doesn't make a difference). The onChange function should not fire multiple times either, since the resulting color is not changing.
  • Play around with the sliders, the number controls, and the 2d picker. The UI should update as expected, and the onChange callback should be fired as expected.

Screenshots or screencast

,

Trunk

Kapture.2023-12-22.at.11.31.40.mp4

This PR

Kapture.2024-01-04.at.23.46.47.mp4

@ciampo ciampo self-assigned this Jan 4, 2024
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Jan 4, 2024
@ciampo ciampo marked this pull request as ready for review January 4, 2024 22:52
@ciampo ciampo requested a review from ajitbohra as a code owner January 4, 2024 22:52
@ciampo ciampo requested review from mirka, tyxla, a team and andrewserong January 5, 2024 08:20
@ciampo ciampo force-pushed the fix/color-picker-hsl-sliders branch from 64d48bd to 4161915 Compare January 5, 2024 08:36
Copy link
Contributor

@andrewhayward andrewhayward left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this is an artefact of colord, but it feels really awkward that we drop the hue and saturation values in some circumstances; is there any way we can maintain them a little more consistently?

I know that H is irrelevant when S is 0, and both are irrelevant when L is {0,100}, but the UX of dropping values just because the colour system doesn't need them isn't great.

Not a blocker for this PR, but something to think about.

packages/components/src/color-picker/hsl-input.tsx Outdated Show resolved Hide resolved
@ciampo
Copy link
Contributor Author

ciampo commented Jan 5, 2024

I'm guessing this is an artefact of colord.
...
Not a blocker for this PR, but something to think about.

I 100% share the sentiment.

ColorPicker's color is expected to be a string. So far we have always passed a hex string, but maybe we could also pass CSS strings with other formats (ie. HSL and RGB). I'll try to timebox an effort and see if we can use colord only when strictly necessary (eg. converting the selected color internally when the used picks a different color space).

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a much better user experience, thanks. ❤️

Would be worth optimizing the useEffect(), and one way to do it could be to rely on the string representation of the HSL instead of on the HSL object. Could be done separately in a follow-up if you prefer.

It would also be nice to add some tests. Could be similar to what you demonstrated on the video preview in the PR description.

@ciampo ciampo force-pushed the fix/color-picker-hsl-sliders branch from 4161915 to cb3303d Compare January 10, 2024 18:11
@ciampo ciampo requested a review from tyxla January 10, 2024 18:28
@ciampo
Copy link
Contributor Author

ciampo commented Jan 10, 2024

Feedback should be addressed. @chad1008 may be also taking a look and smoke test color pickers around the editor.

I've also started exploring a more general solution which affects the whole ColorPicker component, but it's definitely a bigger piece of work than this PR. I may timebox some more time, but likely I'll have to drop it in order to prioritize more urgent work

@tyxla
Copy link
Member

tyxla commented Jan 11, 2024

Still looking great to me, thanks!

@ciampo ciampo requested a review from chad1008 January 11, 2024 14:18
Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good and tests well for me, both in Storybook and in the editor! 🚀 🚢

@ciampo ciampo merged commit 4f97a3f into trunk Jan 11, 2024
58 checks passed
@ciampo ciampo deleted the fix/color-picker-hsl-sliders branch January 11, 2024 16:48
@github-actions github-actions bot added this to the Gutenberg 17.6 milestone Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

ColorPicker: sliders stuck on 0 value when editing HSL color value
4 participants