feat(notifications): new light/dark mode colors #1842

geotrev · 2024-06-18T22:23:46Z

Description

Components updated in this PR:

Detail

Internal prop spreads have been converted to transient props, with the exception of props used by non-converted components (e.g., GlobalAlert.Button retains all props from its base Button).

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

geotrev · 2024-06-18T22:32:22Z

packages/notifications/src/styled/global-alert/StyledGlobalAlert.ts

+  switch ($alertType) {
+    case 'success': {
+      borderColor = getColor({
+        variable: 'border.successEmphasis',


Interested in thoughts around this use of emphasis vs non-emphasis. In general, I tried to use non-emphasis with foreground and emphasis with background. Does this align with design intent?

Note this doesn't apply across all components, and at some points foreground is using emphasis (see StyledAlert).

I'd like to ensure that when a consumer updates a color variable, it cascades through these components in the least confusing way.

Also, let me know if I'm overthinking it. 😛

coveralls · 2024-06-18T22:55:32Z

coverage: 96.035% (-0.008%) from 96.043%
when pulling 8991351 on george/notifications-recolor
into 54514f3 on next.

coveralls · 2024-06-20T01:57:14Z

coverage: 96.035% (-0.008%) from 96.043%
when pulling be05ccb on george/notifications-recolor
into 54514f3 on next.

coveralls · 2024-06-20T15:19:42Z

coverage: 96.035% (-0.008%) from 96.043%
when pulling 1b81444 on george/notifications-recolor
into 54514f3 on next.

steue · 2024-06-20T17:24:12Z

thanks @geotrev!! for posterity from our IC sync

default well in dark mode should have "default" styling, then "raised" styling when isFloating

~~2. design is in agreement to "fix" button styling so warning button treatment uses yellow-700.~~ looks like you tackled that already!

other than that it looks good!

coveralls · 2024-06-20T17:39:13Z

coverage: 96.035% (-0.008%) from 96.043%
when pulling 6650cf4 on george/notifications-recolor
into 54514f3 on next.

jzempel · 2024-06-20T16:49:38Z

packages/notifications/src/styled/StyledBase.ts

+  'data-garden-id': COMPONENT_ID,
+  'data-garden-version': PACKAGE_VERSION


I know I said all Styled files need retrieveComponentStyles, but I wonder if this particular one is desirable given that it is always meant to be extended. I suppose this doesn't hurt, but is it necessary?

I remember we had some back and forth originally, and we decided to leave out the details of the base component because we expected all other icon components (like this one) to define the COMPONENT_ID AND retrieveComponentStyles etc.

jzempel · 2024-06-20T19:31:05Z

packages/notifications/src/elements/content/Close.tsx

    return (
-      <StyledClose ref={ref} hue={hue} aria-label={ariaLabel} {...props}>
+      <StyledClose ref={ref} $type={type} aria-label={ariaLabel} {...props} focusInset size="small">
        <XStrokeIcon />


Do we get better results with the 16px? The 12px is looking unintentionally heavy here.

For comparison:

12px

16px

packages/notifications/src/elements/global-alert/GlobalAlertButton.tsx

packages/notifications/src/styled/StyledBase.ts

jzempel · 2024-06-20T19:48:44Z

packages/notifications/src/styled/global-alert/StyledGlobalAlert.ts

+const lightDarkOptions = (lightOffset: number, darkOffset: number) => ({
+  light: { offset: lightOffset },
+  dark: { offset: darkOffset }
+});


I thought the design directive for GlobalAlert was to go with one set of colors, regardless of light/dark mode. Hopefully the color logic below can be simplified.

Can you clarify what you are hoping to see / what isn't aligning for you? What are you hoping to see in terms of simplification?

Steph, Lucijan and I had a brief exchange about using variables here instead of the raw colors as the semantic importance of them fits well with GlobalAlert (success, error, warning, etc). For instance, if a consumer updates background.successEmphasis from green to blue, wouldn't we expect GA to reflect the new success color for its success type?

Unless of course I've been misunderstanding the intent of GA this entire time and there is preference to directly reference the colors without variables (in order to ensure the same palette is used regardless of what variables are set). 🤔

jzempel · 2024-06-20T19:51:52Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertButton.ts

+  let offsetOptions: OffsetOptions = { light: { offset: 200 }, dark: { offset: 300 } };
+  let offsetHoverOptions: OffsetOptions = { light: { offset: 300 }, dark: { offset: 400 } };
+  let offsetActiveOptions: OffsetOptions = { light: { offset: 400 }, dark: { offset: 500 } };


Repeating from the GlobalAlert comment, I don't think we should be seeing light/dark offsets. The GlobalAlert.Button functions the same regardless of mode.

Similar to my comment on the other comment, the offsets are there to ensure the underlying shade remains consistent between light/dark. If that isn't going to work here, we should definitely discuss in more detail.

The direction I heard from @lucijanblagonic and @steue is that the components on the surface of a GlobalAlert do not change color (or transition hover/active state) differently in light & dark modes. I'm sure we want to use variables. But I wasn't expecting to see the light and dark parameters in use for getColor here.

Oh, I see what you mean. The reason is that the values used in those variables between light and dark already differ, so they need to be offset to meet the consistent shade in either mode. Hopefully I'm understanding correctly.

jzempel · 2024-06-20T19:53:04Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertClose.ts

+        light: { offset: 200 },
+        dark: { offset: 500 },
+        theme
+      });
+      activeBackgroundColor = getColor({
+        variable: 'background.warningEmphasis',
+        transparency: theme.opacity[200],
+        theme
+      });
+      activeForegroundColor = getColor({
+        variable: 'foreground.warningEmphasis',
+        light: { offset: 300 },
+        dark: { offset: 600 },


Ditto re: modeless on GlobalAlert.Close.

jzempel · 2024-06-20T19:54:01Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertIcon.ts

+        light: { offset: -400 },
+        dark: { offset: -100 },
+        theme
+      });
+      break;
+    case 'error':
+      // red/300
+      color = getColor({
+        variable: 'foreground.danger',
+        light: { offset: -400 },
+        dark: { offset: -100 },


Also modeless for the GlobalAlert icon.

jzempel · 2024-06-20T19:54:54Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertTitle.ts

+      color = getColor({ variable: 'foreground.warningEmphasis', dark: { offset: 600 }, theme });
      break;

    case 'info':
-      color = getColorV8('primaryHue', 800, props.theme);
+      color = getColor({
+        variable: 'foreground.primary',
+        light: { offset: 200 },
+        dark: { offset: 300 },


Ditto re: modeless for GlobalAlert.Title.

ze-flo · 2024-06-20T20:22:30Z

packages/notifications/src/utils/icons.ts

+export const validationTypes: Record<Type, string> = {
+  success: 'success',
+  error: 'error',
+  warning: 'warning',
+  info: 'info'


ze-flo · 2024-06-20T20:33:54Z

packages/notifications/src/utils/useNotificationsContext.ts

-export type Hue = 'successHue' | 'warningHue' | 'dangerHue' | 'neutralHue';
-
-export const NotificationsContext = createContext<Hue | undefined>(undefined);
+export const NotificationsContext = createContext<Type | undefined>(undefined);


[nit] The props.type is required, thus should always defined.

To align the context API with the props', should we specify a default rather than using undefined?

Suggested change

export const NotificationsContext = createContext<Type | undefined>(undefined);

export const NotificationsContext = createContext<Type | undefined>('info');

Doing so will make things a bit clearer in that file:

https://github.com/zendeskgarden/react-components/pull/1842/files#diff-30a97b0e497db8834422a4c3a59fd93ac89d538cb7fc5a65d34f0f9be4850c73R17

https://github.com/zendeskgarden/react-components/pull/1842/files#diff-30a97b0e497db8834422a4c3a59fd93ac89d538cb7fc5a65d34f0f9be4850c73R29

I think I need to look into this more. I'm suspicious the issue is actually because Alert and Notifications share view subcomponent (Close, Title, etc), but only Alert requires type, so it being undefined might be a valid scenario. I'll check it out.

OK, I confirmed that in all places where type is not required (e.g., subcomponents), there are fall backs. The rest of the components seem to handle the required prop appropriately.

Thanks for clarifying. 💯

coveralls · 2024-06-21T16:23:12Z

coverage: 96.015% (-0.03%) from 96.043%
when pulling 1be280c on george/notifications-recolor
into 54514f3 on next.

coveralls · 2024-06-24T17:09:27Z

coverage: 96.023% (-0.02%) from 96.043%
when pulling 9244fe1 on george/notifications-recolor
into 54514f3 on next.

jzempel · 2024-06-24T18:59:55Z

packages/notifications/src/styled/StyledBase.ts

+  const boxShadow = shadows.lg(offsetY, blurRadius, color);
+
  return css`
    border-color: ${borderColor};
+    box-shadow: ${$isFloating && boxShadow};


Suggested change

const boxShadow = shadows.lg(offsetY, blurRadius, color);

return css`

border-color: ${borderColor};

box-shadow: ${$isFloating && boxShadow};

const boxShadow = $isFloating ? shadows.lg(offsetY, blurRadius, color) : undefined;

return css`

border-color: ${borderColor};

box-shadow: ${boxShadow};

[nit] could save an unnecessary function call here.

Fixed :) ec91f22

jzempel · 2024-06-24T19:05:49Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertButton.ts

+  let offsetOptions: OffsetOptions = { light: { offset: 200 }, dark: { offset: 300 } };
+  let offsetHoverOptions: OffsetOptions = { light: { offset: 300 }, dark: { offset: 400 } };
+  let offsetActiveOptions: OffsetOptions = { light: { offset: 400 }, dark: { offset: 500 } };


The direction I heard from @lucijanblagonic and @steue is that the components on the surface of a GlobalAlert do not change color (or transition hover/active state) differently in light & dark modes. I'm sure we want to use variables. But I wasn't expecting to see the light and dark parameters in use for getColor here.

coveralls · 2024-06-25T17:11:28Z

coverage: 96.023% (-0.02%) from 96.043%
when pulling ec91f22 on george/notifications-recolor
into 54514f3 on next.

coveralls · 2024-06-25T21:30:27Z

coverage: 96.019% (-0.02%) from 96.043%
when pulling 6b16bab on george/notifications-recolor
into 54514f3 on next.

packages/notifications/src/elements/global-alert/GlobalAlert.tsx

Co-authored-by: Florent <ze-flo@users.noreply.github.com>

coveralls · 2024-06-25T21:56:51Z

coverage: 96.024% (-0.03%) from 96.057%
when pulling 0bcf703 on george/notifications-recolor
into ee0e4f5 on next.

jzempel · 2024-06-27T16:16:54Z

packages/notifications/src/styled/global-alert/StyledGlobalAlert.ts

+  if (['error', 'success'].includes($alertType)) {
+    const borderVariable =
+      $alertType === 'success' ? 'border.successEmphasis' : 'border.dangerEmphasis';
+    const backgroundVariable =
+      $alertType === 'success' ? 'background.successEmphasis' : 'background.dangerEmphasis';
+    const foregroundVariable =
+      $alertType === 'success' ? 'foreground.success' : 'foreground.danger';
+
+    borderColor = getColor({ variable: borderVariable, light: { offset: 100 }, theme });
+    backgroundColor = getColor({ variable: backgroundVariable, theme });
+    foregroundColor = getColor({ variable: foregroundVariable, light: { offset: -600 }, theme });
+    focusVariable =
+      $alertType === 'success' ? 'foreground.successEmphasis' : 'foreground.dangerEmphasis';
+    anchorHoverColor = theme.palette.white;
+    anchorActiveColor = theme.palette.white;
+  } else {
+    const borderVariable =
+      $alertType === 'warning' ? 'border.warningEmphasis' : 'border.primaryEmphasis';
+    const backgroundVariable =
+      $alertType === 'warning' ? 'background.warningEmphasis' : 'background.primaryEmphasis';
+    const foregroundVariable =
+      $alertType === 'warning' ? 'foreground.warning' : 'foreground.primary';


Are these ternaries buying us much? Feels like the previous switch was more readable.

Reverted. :)

jzempel · 2024-06-27T16:19:03Z

packages/notifications/src/styled/global-alert/StyledGlobalAlert.ts

+    borderColor = getColor({ variable: borderVariable, light: { offset: -300 }, theme });
+    backgroundColor = getColor({ variable: backgroundVariable, light: { offset: -400 }, theme });
+    foregroundColor = getColor({ variable: foregroundVariable, light: { offset: 100 }, theme });
+    anchorHoverColor = getColor({ variable: foregroundVariable, light: { offset: 200 }, theme });
+    anchorActiveColor = getColor({ variable: foregroundVariable, light: { offset: 300 }, theme });


I think it would be better to use the offset: xx parameter throughout rather than light: { offset: xx } as the intent is to adjust theme-agnostic (which we know is now always forced to light).

jzempel · 2024-06-27T16:21:12Z

packages/notifications/src/styled/global-alert/StyledGlobalAlert.ts

  'data-garden-id': COMPONENT_ID,
  'data-garden-version': PACKAGE_VERSION
-})`
+})<IStyledGlobalAlertProps>`


Is this type parameter required?

Just so the $alertType prop is valid in the elements JSX side.

jzempel · 2024-06-27T16:23:07Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertButton.ts

+  let offsetOptions: OffsetOptions;
+  let offsetHoverOptions: OffsetOptions;
+  let offsetActiveOptions: OffsetOptions;


Suggested change

let offsetOptions: OffsetOptions;

let offsetHoverOptions: OffsetOptions;

let offsetActiveOptions: OffsetOptions;

let offset: number;

let offsetHover: number;

let offsetActive: number;

...again, I think just using the getColor({ theme, offset: <number> }) is the better way to go, and easier to read/maintain.

jzempel · 2024-06-27T16:24:40Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertClose.ts

+  if (['success', 'error'].includes($alertType)) {
+    const variable = $alertType === 'success' ? 'background.success' : 'background.danger';
+    focusVariable =
+      $alertType === 'success' ? 'foreground.successEmphasis' : 'foreground.dangerEmphasis';
+
+    hoverBackgroundColor = getColor({ variable, theme, transparency: theme.opacity[100] });
+    hoverForegroundColor = theme.palette.white;
+    activeBackgroundColor = getColor({ variable, theme, transparency: theme.opacity[200] });
+    activeForegroundColor = theme.palette.white;
+  } else if ($alertType === 'warning') {
+    const bgVariable = 'background.warningEmphasis';
+    const foregroundVariable = 'foreground.warningEmphasis';
+    focusVariable = 'foreground.warning';
+
+    hoverBackgroundColor = getColor({
+      variable: bgVariable,
+      transparency: theme.opacity[100],
+      theme
+    });
+    hoverForegroundColor = getColor({
+      variable: foregroundVariable,
+      light: { offset: 200 },
+      theme
+    });
+    activeBackgroundColor = getColor({
+      variable: bgVariable,
+      transparency: theme.opacity[200],
+      theme
+    });
+    activeForegroundColor = getColor({
+      variable: foregroundVariable,
+      light: { offset: 300 },
+      theme
+    });
+  } else {
+    focusVariable = 'foreground.primary';
  }


ditto re: ternaries .. the previous switch feels more readable/straightforward to process.

jzempel · 2024-06-27T16:25:53Z

packages/notifications/src/styled/global-alert/StyledGlobalAlertIcon.ts

+        light: { offset: -400 },
+        theme
+      });
+      break;
+    case 'error':
+      // red/300
+      color = getColor({
+        variable: 'foreground.danger',
+        light: { offset: -400 },


Suggested change

light: { offset: -400 },

theme

});

break;

case 'error':

// red/300

color = getColor({

variable: 'foreground.danger',

light: { offset: -400 },

offset: -400,

theme

});

break;

case 'error':

// red/300

color = getColor({

variable: 'foreground.danger',

offset: -400,

packages/notifications/src/styled/StyledBase.ts

steue · 2024-06-27T17:37:56Z

thanks for the fixes! visually looks good to me!

jzempel

🙌

coveralls · 2024-06-27T19:22:28Z

coverage: 96.024% (-0.03%) from 96.057%
when pulling 11836cc on george/notifications-recolor
into ee0e4f5 on next.

Co-authored-by: Florent <ze-flo@users.noreply.github.com>

ze-flo

So many colors! 🤩 🟥 🟢 🟨 🟦 🌈

coveralls · 2024-06-27T19:55:12Z

coverage: 96.024% (-0.03%) from 96.057%
when pulling 9067ce0 on george/notifications-recolor
into ee0e4f5 on next.

geotrev added 5 commits June 14, 2024 13:31

feat(notifications): new light/dark mode colors

23bb0e4

feat(notifications): updates tests

83325f5

chore: adds colors for alert

5025b93

chore: adds colors for global alert

07cbe24

chore: adds colors to Well

b05e87a

geotrev added the PR: New Feature 🚀 label Jun 18, 2024

geotrev self-assigned this Jun 18, 2024

geotrev requested a review from a team as a code owner June 18, 2024 22:23

geotrev commented Jun 18, 2024

View reviewed changes

chore: type import in tests

8991351

zendesk-garden deployed to staging June 18, 2024 22:56 View deployment

chore: adds styled base container light/dark tests

00a21ef

geotrev force-pushed the george/notifications-recolor branch from 4b8b9f6 to 00a21ef Compare June 20, 2024 01:40

chore: remove unused validationHues enum

be05ccb

zendesk-garden deployed to staging June 20, 2024 01:58 View deployment

chore: adds color tests for views

1b81444

zendesk-garden deployed to staging June 20, 2024 15:19 View deployment

chore: changes to global alert button colors (info and warning)

6650cf4

zendesk-garden deployed to staging June 20, 2024 17:40 View deployment

jzempel mentioned this pull request Jun 20, 2024

feat(modals): new light/dark mode colors for TooltipModal and Drawer #1840

Merged

7 tasks

jzempel requested changes Jun 20, 2024

View reviewed changes

ze-flo reviewed Jun 20, 2024

View reviewed changes

chore: code review

1be280c

zendesk-garden deployed to staging June 21, 2024 16:24 View deployment

zendesk-garden deployed to staging June 24, 2024 17:06 View deployment

jzempel reviewed Jun 24, 2024

View reviewed changes

chore: code reviewwwwww

ec91f22

zendesk-garden deployed to staging June 25, 2024 17:12 View deployment

chore: force light mode, remove dark offsets, simplify tests

6b16bab

zendesk-garden deployed to staging June 25, 2024 21:31 View deployment

ze-flo reviewed Jun 25, 2024

View reviewed changes

packages/notifications/src/elements/global-alert/GlobalAlert.tsx Outdated Show resolved Hide resolved

chore: clean up switch in global alert view

eeedb01

geotrev force-pushed the george/notifications-recolor branch from d56fd54 to 01cffd2 Compare June 25, 2024 21:39

chore: use parent theme callback

0bcf703

Co-authored-by: Florent <ze-flo@users.noreply.github.com>

geotrev force-pushed the george/notifications-recolor branch from 01cffd2 to 0bcf703 Compare June 25, 2024 21:39

zendesk-garden deployed to staging June 25, 2024 21:57 View deployment

jzempel reviewed Jun 27, 2024

View reviewed changes

ze-flo reviewed Jun 27, 2024

View reviewed changes

packages/notifications/src/styled/StyledBase.ts Outdated Show resolved Hide resolved

chore: code review

11836cc

geotrev requested a review from jzempel June 27, 2024 19:05

jzempel approved these changes Jun 27, 2024

View reviewed changes

zendesk-garden deployed to staging June 27, 2024 19:21 View deployment

chore: replace Object.keys

9067ce0

Co-authored-by: Florent <ze-flo@users.noreply.github.com>

ze-flo approved these changes Jun 27, 2024

View reviewed changes

zendesk-garden deployed to staging June 27, 2024 19:55 View deployment

geotrev merged commit f5ed1a9 into next Jun 27, 2024

geotrev deleted the george/notifications-recolor branch June 27, 2024 19:59

		'data-garden-id': COMPONENT_ID,
		'data-garden-version': PACKAGE_VERSION

	export const NotificationsContext = createContext<Type \| undefined>(undefined);
	export const NotificationsContext = createContext<Type \| undefined>('info');

feat(notifications): new light/dark mode colors #1842

feat(notifications): new light/dark mode colors #1842

Uh oh!

Conversation

geotrev commented Jun 18, 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!

coveralls commented Jun 18, 2024

Uh oh!

coveralls commented Jun 20, 2024

Uh oh!

coveralls commented Jun 20, 2024

Uh oh!

steue commented Jun 20, 2024

Uh oh!

coveralls commented Jun 20, 2024

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

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

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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 21, 2024

Uh oh!

coveralls commented Jun 24, 2024

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

geotrev commented Jun 18, 2024 •

edited

Loading

geotrev Jun 20, 2024 •

edited

Loading

geotrev Jun 24, 2024 •

edited

Loading

geotrev Jun 20, 2024 •

edited

Loading

geotrev Jun 21, 2024 •

edited

Loading