feat(avatars): adds light/dark mode colors #1852

geotrev · 2024-07-10T20:19:14Z

Description

Updates Avatar and StatusIndicator with new color palette.

Detail

Also updates pattern examples to use Menu.

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-07-11T13:59:27Z

Link to deployment

geotrev · 2024-07-11T15:45:31Z

packages/avatars/src/styled/StyledStatusIndicatorBase.ts

+const colorStyles = ({ theme, type }: IStyledStatusIndicatorProps) => {
+  const foregroundColor = getColor({
+    light: { hue: 'white' },
+    dark: { hue: 'neutralHue', shade: 1100 },


Feels clunky, but it was in the design... waiting for their feedback to confirm better alternatives.

packages/avatars/demo/~patterns/stories/MenuStory.tsx

packages/avatars/src/elements/Avatar.spec.tsx

packages/avatars/src/styled/StyledAvatar.spec.tsx

packages/avatars/src/styled/StyledStatusIndicator.spec.tsx

packages/avatars/src/styled/utility.ts

jzempel · 2024-07-12T15:19:51Z

packages/avatars/src/styled/StyledStatusIndicator.ts

  let boxShadow = theme.shadows.sm(
-    surfaceColor ||
-      (type
-        ? getColorV8('background', 600 /* default shade */, theme)!
+    $surfaceColor ||
+      ($type
+        ? getColor({ variable: 'background.default', theme })
        : (theme.palette.white as string))
  );

-  if (size === xxs) {
+  if ($size === xxs) {
    boxShadow = boxShadow.replace(theme.shadowWidths.sm, '1px');
  }


Potential performance improvement by setting let boxShadow; undefined and moving the theme.shadows.sm to a conditional else clause here.

Not sure I'm seeing the gain. We calculate the box shadow either way, don't we? 🤔

On second thought, I'm not actually entirely sure what you mean by moving the theme.shadows.sm to an else clause. Can you elaborate?

Ah, I missed the replace. That's pretty odd treatment for our typical goto. I think the code should follow normative Garden conventions and look more like this...

const boxShadowColor = $surfaceColor || getColor({ theme, variable: 'background.default' }) let boxShadow; if (size === xxs) { boxShadow = theme.shadows.xs(boxShadowColor); } else { boxShadow = theme.shadows.sm(boxShadowColor); }

Stepping away from the theme like the current code does isn't ideal.

Either that or make the boxShadow a simple ternary...

const boxShadow = size === xxs ? theme.shadows.xs(boxShadowColor) : theme.shadows.sm(boxShadowColor);

That makes total sense! Thanks for clarifying.

jzempel · 2024-07-12T15:20:56Z

packages/avatars/src/styled/StyledStatusIndicatorBase.ts

+  let backgroundColor = getStatusColor(theme, $type);
  let borderColor = backgroundColor;


Suggested change

let backgroundColor = getStatusColor(theme, $type);

let borderColor = backgroundColor;

let backgroundColor;

let borderColor;

...and move assignments to a conditional else clause below.

This feels like a nit, but I guess I'm not seeing a gain in clarity with moving the assignment down a few lines when the variables can only be one of two values anyway?

Could be a [nit], but in practice Garden tends to micro-optimize by deferring conditional assignment (and associated function call expense) until they are determined to be necessary. More like...

let backgroundColor; let borderColor; if ($type === 'offline') { backgroundColor = ...; borderColor = ...; } else { backgroundColor = ...; borderColor = ...; }

I guess that's the nit I'm trying to understand. Whether the value is assigned at the initial assignment aka default value (let thing = value) or at the else (which is acting as the default value), this feels like a stylistic preference, right? I just want to make sure I understand that that's what we're talking about, because I'm not seeing any real optimization in this scenario. 🙂

Okay, I think I understand now after looking at it for a minute. I didn't see initially that getColor would be called extraneously in the current code. I'll update and push it up!

ze-flo · 2024-07-12T18:26:11Z

packages/avatars/src/styled/utility.ts

-  switch (type) {
-    case 'active':
-      return getColorV8('crimson', 400, theme)!;
-    case 'available':
-      return getColorV8('mint', 400, theme)!;
-    case 'away':
-      return getColorV8('orange', 400, theme)!;
-    case 'transfers':
-      return getColorV8('azure', 400, theme)!;
-    case 'offline':
-      return getColorV8('grey', 500, theme)!;
-    default:
-      return 'transparent';
-  }


Def a [nit] and related to the micro-optimization comment above, the switch statement is slightly more performant then calculating all colors then picking the right one.

Let me know if I captured the intent of this optimization in my latest commit. I think I did but after reading your comment I'm not entirely sure. 😅

Yeah, I was more thinking the map would live in a one-time constant. And the function call would be more along the lines of

react-components/packages/chrome/src/styled/utils.ts

Lines 24 to 25 in 816c29f

export const getProductColor = (product?: Product, fallback = 'inherit') =>

product ? PALETTE.product[product] || fallback : fallback;

The one time constant would def work.

What I wanted to emphasize is that when using a map 5 values are calculated every time getStatusColor is called but only one color is returned. With a switch statement, however, there's one calculation for one returned color. It's more efficient.

Can you be more specific @jzempel ? These functions feel very different to me.

I dig it! 💯

Hey @geotrev maybe something like this?

const STATUS_COLOR_OPTIONS: Record< NonNullable<IStyledStatusIndicatorProps['$type']>, Omit<ColorParameters, 'theme'> > = { active: { hue: 'crimson', light: { shade: 700 }, dark: { shade: 600 } }, available: { hue: 'mint', light: { shade: 500 }, dark: { shade: 400 } }, away: { hue: 'orange', light: { shade: 500 }, dark: { shade: 400 } }, transfers: { hue: 'azure', light: { shade: 500 }, dark: { shade: 400 } }, offline: { hue: 'grey', light: { shade: 500 }, dark: { shade: 400 } } }; export function getStatusColor( theme: IStyledStatusIndicatorProps['theme'], type?: IStyledStatusIndicatorProps['$type'] ): string { return type ? getColor({ ...STATUS_COLOR_OPTIONS[type], theme }) || 'transparent' : 'transparent'; }

Haha, looks like we arrived at a similar result 😉

Two peas 😎

I like retaining the 'transparent' fallback in case the color enum lookup returns undefined, so I added that in. :)

steue

visually looks good to me! thnx for working on this so meticulously! 🔥 🚀

jzempel

All good on my end, pending resolution to @ze-flo's optimization comment

ze-flo · 2024-07-12T19:22:41Z

packages/avatars/src/styled/utility.ts

-  switch (type) {
-    case 'active':
-      return getColorV8('crimson', 400, theme)!;
-    case 'available':
-      return getColorV8('mint', 400, theme)!;
-    case 'away':
-      return getColorV8('orange', 400, theme)!;
-    case 'transfers':
-      return getColorV8('azure', 400, theme)!;
-    case 'offline':
-      return getColorV8('grey', 500, theme)!;
-    default:
-      return 'transparent';
-  }


The one time constant would def work.

What I wanted to emphasize is that when using a map 5 values are calculated every time getStatusColor is called but only one color is returned. With a switch statement, however, there's one calculation for one returned color. It's more efficient.

ze-flo · 2024-07-12T19:27:52Z

packages/avatars/src/styled/utility.ts

+  return (
+    {
+      active: getColor({ hue: 'crimson', light: { shade: 700 }, dark: { shade: 600 }, theme }),
+      available: getColor({ hue: 'mint', light: { shade: 500 }, dark: { shade: 400 }, theme }),
+      away: getColor({ hue: 'orange', light: { shade: 500 }, dark: { shade: 400 }, theme }),
+      transfers: getColor({ hue: 'azure', light: { shade: 500 }, dark: { shade: 400 }, theme }),
+      offline: getColor({ hue: 'grey', light: { shade: 500 }, dark: { shade: 400 }, theme })
+    }[type as string] || 'transparent'
+  );


Suggested change

return (

{

active: getColor({ hue: 'crimson', light: { shade: 700 }, dark: { shade: 600 }, theme }),

available: getColor({ hue: 'mint', light: { shade: 500 }, dark: { shade: 400 }, theme }),

away: getColor({ hue: 'orange', light: { shade: 500 }, dark: { shade: 400 }, theme }),

transfers: getColor({ hue: 'azure', light: { shade: 500 }, dark: { shade: 400 }, theme }),

offline: getColor({ hue: 'grey', light: { shade: 500 }, dark: { shade: 400 }, theme })

}[type as string] || 'transparent'

);

switch (type) {

case 'active':

return getColor({ hue: 'crimson', light: { shade: 700 }, dark: { shade: 600 }, theme });

case 'available':

return getColor({ hue: 'mint', light: { shade: 500 }, dark: { shade: 400 }, theme });

case 'away':

return getColor({ hue: 'mint', light: { shade: 500 }, dark: { shade: 400 }, theme });

case 'transfers':

return getColor({ hue: 'orange', light: { shade: 500 }, dark: { shade: 400 }, theme });

case 'offline':

return getColor({ hue: 'grey', light: { shade: 500 }, dark: { shade: 400 }, theme });

default:

return 'transparent';

}

Ah, that's what I had before.

I feel like there are two different trains of thought here. I ended up doing something like this: 403bf68

ze-flo

Very nice! 🚀

jzempel · 2024-07-12T20:31:11Z

packages/avatars/src/styled/utility.ts

+
+  const colorArgs = StatusColorParams[type];
+
+  return getColor({ ...colorArgs, theme });


Suggested change

return getColor({ ...colorArgs, theme });

return getColor({ ...colorArgs, theme }) || 'transparent';

The only thing is that there might be an unprotected path where a consumer provides a rogue status type value 🤷. Might not be worth sealing off.

You beat me to it. Just pushed that up. I did something similar, but I think both could be valid. I'm OK switching to you what here if you think that's worth it.

jzempel

i dig it 🎉

geotrev added the PR: New Feature 🚀 label Jul 10, 2024

geotrev self-assigned this Jul 10, 2024

geotrev requested a review from a team as a code owner July 10, 2024 20:19

geotrev commented Jul 11, 2024

View reviewed changes

geotrev added 4 commits July 12, 2024 09:56

feat(avatars): adds light/dark mode colors

f7e8416

chore: corrects foreground of status indicator

de96047

chore: use transient props

ad87795

chore: design color fix

462d84d

geotrev force-pushed the george/recolor-avatars branch from 872f86d to 462d84d Compare July 12, 2024 14:56

jzempel requested changes Jul 12, 2024

View reviewed changes

chore: code review changes

9cce9ad

ze-flo reviewed Jul 12, 2024

View reviewed changes

geotrev requested review from steue and a team July 12, 2024 18:30

steue approved these changes Jul 12, 2024

View reviewed changes

chore: code review

9c2c239

jzempel approved these changes Jul 12, 2024

View reviewed changes

ze-flo reviewed Jul 12, 2024

View reviewed changes

chore: code review getStatusColor

403bf68

ze-flo approved these changes Jul 12, 2024

View reviewed changes

jzempel reviewed Jul 12, 2024

View reviewed changes

chore: use transparent fallback

1b86420

jzempel approved these changes Jul 12, 2024

View reviewed changes

github-actions bot deployed to staging July 12, 2024 20:42 View deployment

geotrev merged commit 58e3875 into next Jul 12, 2024

geotrev deleted the george/recolor-avatars branch July 12, 2024 20:47

		let backgroundColor = getStatusColor(theme, $type);
		let borderColor = backgroundColor;

	export const getProductColor = (product?: Product, fallback = 'inherit') =>
	product ? PALETTE.product[product] \|\| fallback : fallback;


		const colorArgs = StatusColorParams[type];

		return getColor({ ...colorArgs, theme });

feat(avatars): adds light/dark mode colors #1852

feat(avatars): adds light/dark mode colors #1852

Uh oh!

Conversation

geotrev commented Jul 10, 2024

Description

Detail

Checklist

Uh oh!

geotrev commented Jul 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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 Jul 12, 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!

Choose a reason for hiding this comment

Uh oh!

geotrev Jul 12, 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!

geotrev Jul 12, 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!

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!

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!

ze-flo left a comment

Choose a reason for hiding this comment

jzempel Jul 12, 2024 •

edited

Loading

geotrev Jul 12, 2024 •

edited

Loading

geotrev Jul 12, 2024 •

edited

Loading

jzempel Jul 12, 2024 •

edited

Loading