feat(colorpickers): adds light/dark mode colors #1862

ze-flo · 2024-07-17T17:58:25Z

Description

Adds dark / light mode support to @zendeskgarden/react-colorpickers

Detail

Adds dark / light mode support to ColorPicker, ColorPickerDialog, ColorSwatch, and ColorSwatchDialog
Simplify CSS with isOpaque option
Use transient props to avoid invalid DOM attributes

Checklist

👌 design updates will be Garden Designer approved (add the designer as a reviewer)
🌐 demo is up-to-date (npm start)
⬅️ renders as expected with reversed (RTL) direction
~~🤘 renders as expected with Bedrock CSS (?bedrock)~~
💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
♿ tested for WCAG 2.1 AA accessibility compliance
📝 tested in Chrome, Firefox, Safari, and Edge

ze-flo · 2024-07-17T18:08:41Z

packages/colorpickers/src/styled/common/StyledRange.ts

  const thumbHoverBorderColor = thumbHoverBackgroundColor;

-  return `
-    border-color: ${props.isOpaque && getColorV8('background', 600 /* default shade */, props.theme)};


With the isOpaque option, the border was only used for positioning. The border-color must then match with the background color of the the container the ColorPicker with isOpaque renders into. That can be problematic in some cases.

So I refactored the CSS to remove it.

If this ends up having no issues with the final interaction/element then sounds good 👍

geotrev · 2024-07-18T14:07:52Z

packages/colorpickers/src/types/index.ts

+  $green: number;
+  $blue: number;
+  $alpha?: number;
+}


geotrev

Looking good!

geotrev · 2024-07-18T15:36:03Z

packages/colorpickers/src/styled/ColorPicker/StyledAlphaRange.ts

  style: {
    backgroundSize: 'auto',
    background: background(props)
  },


Out of curiosity, why can't this live in the style block below?

I think this approach avoids dynamically creating a new class every time the color props change. styled-component will warn you when that happens.

...and performance drops through the floor

jzempel

[nit] should the data for the ColorSwatch demos be updated to show all 12 colors from the primary (blue, green, red, yellow) palette?

jzempel · 2024-07-18T17:41:24Z

packages/colorpickers/src/styled/ColorPicker/StyledSliders.ts

-  }
+  ${props =>
+    !props.$isOpaque &&
+    css`


Is css necessary here?

This provides proper placement for the sliders when isOpaque is false.

yep, I get that. More wondering whether this can be a simple template literal rather than invoking the css function?

I tried the template literal first but it didn't parse correctly. I'll give it another shot.

...as I understand it, styled already handles this, making css redundant...

during flattening, styled-components ignores interpolations that evaluate to undefined, null, false, or an empty string ("")

https://styled-components.com/docs/advanced#tagged-template-literals

jzempel · 2024-07-18T17:54:11Z

packages/colorpickers/src/styled/common/StyledRange.ts

-const colorStyles = (props: IStyledRangeProps & ThemeProps<DefaultTheme>) => {
-  const thumbBackgroundColor = getColorV8('neutralHue', 100, props.theme);
+const colorStyles = ({ theme }: ThemeProps<DefaultTheme>) => {
+  const thumbBackgroundColor = getColor({ theme, variable: 'background.default' });


@lucijanblagonic maybe just me, but I'm finding these dark mode thumbs very difficult to see. Somehow I don't have the same problem with shadowed white over white background.

+1, feels like an accessibility concern for sure.

@steue @lucijanblagonic For clarity, we're referring to this:

great observation yall! curious about your thoughts of bumping up the contrast by setting the thumbs to bg/raised with a -100 offset? (looks brighter)

curious about your thoughts of bumping up the contrast by setting the thumbs to bg/raised with a -100 offset? (looks brighter)

@steue for me, that color tweak works great 🙌

Updated. ✅

jzempel · 2024-07-18T17:55:42Z

packages/colorpickers/src/types/index.ts

+export interface IRGBColorProps {
+  $red: number;
+  $green: number;
+  $blue: number;
+  $alpha?: number;
+}


Can this move to a types.ts file within the styled directory? We just wouldn't want to mistake the fact that this interface should never be publicly exported.

steue

sans the thumb treatment, everything looks good to me! great work!

jzempel · 2024-07-19T19:05:03Z

packages/colorpickers/src/styled/ColorPicker/StyledSliders.ts

 }

+const sizeStyles = (props: IStyledSlidersProps & ThemeProps<DefaultTheme>) => {
+  if (props.$isOpaque) return undefined;


Suggested change

if (props.$isOpaque) return undefined;

if (props.$isOpaque) {

return undefined;

}

Conditions in Garden should always be blocks. We used to lint for this, but the rule was deprecated. Probably need to bring it back with a plugin since prettier doesn't cover us.

lucijanblagonic · 2024-07-22T14:16:39Z

The changes here look good, thanks @steue for jumping in. The only thing that seems off is the hover behavior of the handle. I would expect the hover to lighten on hover (in dark mode), to be in line with other interactive controls. What do you think?

steue · 2024-07-22T16:30:23Z

The changes here look good, thanks @steue for jumping in. The only thing that seems off is the hover behavior of the handle. I would expect the hover to lighten on hover (in dark mode), to be in line with other interactive controls. What do you think?

agreed!

ze-flo · 2024-07-22T16:37:49Z

The changes here look good, thanks @steue for jumping in. The only thing that seems off is the hover behavior of the handle. I would expect the hover to lighten on hover (in dark mode), to be in line with other interactive controls. What do you think?

agreed!

Great! In dark mode, the slider's thumb is using background.raised with an offset: -100 for its default state. @steue What offset would you recommend for the hover state?

geotrev · 2024-07-22T17:07:00Z

Not sure the best place to put this so I'll throw it here. 😅

I'm going off of the updated designs:

I'm still worried about this contrast. Even with the box shadow, the thumb blends in with the background especially at the ends of the range (which is the default state).

I'm not sure what the best approach is (border, or maybe flipping the thumb to be white for dark mode and dark grey for light mode). Maybe this isn't a problem we can solve in this PR, which is also fair. But wanted to at least put it our there for posterity.

I also asked Jen over in accessibility and she agreed, FWIW. She shared the criterion, which specifies:

For active controls any visual information provided that is necessary for a user to identify that a control is present and how to operate it must have a minimum 3:1 contrast ratio with the adjacent colors.

This success criterion does not require that controls have a visual boundary indicating the hit area, but if the visual indicator of the control is the only way to identify the control, then that indicator must have sufficient contrast.

steue · 2024-07-22T18:36:22Z

What offset would you recommend for the hover state?

i would imagine following our default on hover, so another -100 (effectively -200)?

steue · 2024-07-22T19:06:02Z

@geotrev valid concern! color pickers have interesting a11y considerations as colors themselves pose inherent issues. we can get into the nitty gritty of it, but where is the contrast compared against? is it against the colors in the slider itself or the BG? you're right, though the the thumb might not always achieve the 3:1 contrast ratio.

that said, the overall design does incorporate alternative methods for controlling color that are accessible. (i.e. thumbs / sliders controllable via keyboard and input fields to control HSL/RGB etc) i do think we could explore treatment as another treatment

ze-flo · 2024-07-22T19:24:09Z

What offset would you recommend for the hover state?

i would imagine following our default on hover, so another -100 (effectively -200)?

Following @lucijanblagonic 's comment:

I would expect the hover to lighten on hover (in dark mode)

@steue If the default state is background.raised with an offset: -100, should we increase the offset by 100 (essentially just background.raised) for hover instead?

geotrev · 2024-07-22T19:33:08Z

@geotrev valid concern! color pickers have interesting a11y considerations as colors themselves pose inherent issues. we can get into the nitty gritty of it, but where is the contrast compared against? is it against the colors in the slider itself or the BG? you're right, though the the thumb might not always achieve the 3:1 contrast ratio.

@steue The contrast is with everything behind it, but primarily the dark grey background. Currently the background and the thumb have less than a 2:1 contrast ratio, which is concerning in the cases I mentioned. Especially since we can't depend on the thumb being positioned inside the slider at all times.

The issue of the contrast not always being perfect could be a good reason to use a border. A single solid color will present persistence issues like you mention.

that said, the overall design does incorporate alternative methods for controlling color that are accessible. (i.e. thumbs / sliders controllable via keyboard and input fields to control HSL/RGB etc) i do think we could explore treatment as another treatment

That is true. There are input fields as well. I bring it up as a medium-high concern mainly because the bulk of the picker UI contains visual/color-based controls.

lucijanblagonic · 2024-07-23T13:19:57Z

I don't think there is a difference in light or dark mode when it comes to accessibility in this case. Having a thumb in the color where it blends with the background shares the same challenge in light and dark. Here is an example with the thumb changes in the slider and in the color well.

Thumb in color well has a white 2px inner border and 1px outer black border.
Thumb in the slider has background/default and then border/input applied. That way the stroke helps to distinguish the handle a bit.

geotrev · 2024-07-23T15:47:54Z

That makes sense and would definitely remediate the concerns. Not a blocker for this PR of course, we can set aside time to do it after if preferred

ze-flo · 2024-07-24T02:50:55Z

@lucijanblagonic @steue The latest preview increases the thumbs contrast based on the last suggestion. The border treatment of the range thumbs matches with the inputs' below.

I think this looks good. Hope this works for all too! 🤞

jzempel · 2024-07-24T13:59:19Z

packages/colorpickers/src/styled/common/StyledRange.ts

+  const thumbBorderColor = getColor({
+    theme,
+    variable: 'border.default',
+    dark: { offset: -100 },
+    light: { offset: 100 }
+  });


[nit] now that the thumbs have a visible border, can we check that the transition animation is updated to include the border?

Transition animations appear to be inherited from Range:

react-components/packages/forms/src/styled/range/StyledRangeInput.ts

Lines 142 to 144 in e1bac19

transition:

border-color .25s ease-in-out,

background-color .25s ease-in-out;

Ok, my bad. I wasn't viewing the :hover state.

lucijanblagonic

Tested and looks good, feel free to merge after resolving @jzempel 's comment for transition.

ze-flo added 4 commits July 16, 2024 13:13

feat(colorpickers): recolor

75445b1

refactor: use transient props

41bfc3f

refactor: clean up

1c78ba9

tests: add specs

7503a72

ze-flo added the PR: New Feature 🚀 label Jul 17, 2024

ze-flo requested a review from a team as a code owner July 17, 2024 17:58

github-actions bot deployed to staging July 17, 2024 18:06 View deployment

ze-flo commented Jul 17, 2024

View reviewed changes

geotrev reviewed Jul 18, 2024

View reviewed changes

packages/colorpickers/src/types/index.ts Outdated

$green: number;

$blue: number;

$alpha?: number;

}

Copy link

Contributor

geotrev Jul 18, 2024

Choose a reason for hiding this comment

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

🔥

geotrev approved these changes Jul 18, 2024

View reviewed changes

geotrev reviewed Jul 18, 2024

View reviewed changes

jzempel reviewed Jul 18, 2024

View reviewed changes

refactor: PR feedback

5c1004a

github-actions bot deployed to staging July 18, 2024 20:26 View deployment

ze-flo requested review from lucijanblagonic and steue July 19, 2024 15:57

steue approved these changes Jul 19, 2024

View reviewed changes

jzempel requested changes Jul 19, 2024

View reviewed changes

chore: PR feedback

5403ac9

github-actions bot deployed to staging July 19, 2024 23:44 View deployment

chore: PR feedback

edc0f11

github-actions bot deployed to staging July 24, 2024 02:45 View deployment

jzempel reviewed Jul 24, 2024

View reviewed changes

lucijanblagonic approved these changes Jul 24, 2024

View reviewed changes

ze-flo added 2 commits July 24, 2024 06:37

chore: remove inherited box-shadow styles + clean up

c2b4cf5

test: update specs

4118715

github-actions bot deployed to staging July 24, 2024 16:46 View deployment

jzempel approved these changes Jul 24, 2024

View reviewed changes

ze-flo merged commit bc7b6b1 into next Jul 24, 2024

ze-flo deleted the ze-flo/color-pickers-recolor branch July 24, 2024 18:23

	transition:
	border-color .25s ease-in-out,
	background-color .25s ease-in-out;

feat(colorpickers): adds light/dark mode colors #1862

feat(colorpickers): adds light/dark mode colors #1862

Uh oh!

Conversation

ze-flo commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Detail

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

geotrev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzempel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucijanblagonic commented Jul 22, 2024

Uh oh!

steue commented Jul 22, 2024

Uh oh!

ze-flo commented Jul 22, 2024

Uh oh!

geotrev commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

steue commented Jul 22, 2024

Uh oh!

steue commented Jul 22, 2024

Uh oh!

ze-flo commented Jul 22, 2024

Uh oh!

geotrev commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucijanblagonic commented Jul 23, 2024

Uh oh!

geotrev commented Jul 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

ze-flo commented Jul 17, 2024 •

edited

Loading

geotrev commented Jul 22, 2024 •

edited

Loading

geotrev commented Jul 22, 2024 •

edited

Loading

geotrev commented Jul 23, 2024 •

edited

Loading