feat(typography): new light/dark mode colors (excluding CodeBlock) #1820

ze-flo · 2024-06-01T00:25:10Z

Description

Adds light / dark mode support for our Typography components, excluding CodeBlock

Detail

Note

~~StyledFont defaults to foreground.default color if no hue is provided. The following components extend it and now inherit the default color:~~
~~- SM~~
~~- MD~~
~~- LG~~
~~- XL~~
~~- XXL~~
~~- XXXL~~
~~- Blockquote~~
~~- OrderedList~~
~~- UnorderedList~~
~~- Span~~

~~Ellipsis and Paragraph's color also defaults to foreground.default.~~

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

packages/tooltips/src/elements/Tooltip.spec.tsx

packages/typography/src/styled/StyledCode.spec.tsx

ze-flo · 2024-06-01T00:28:49Z

packages/typography/src/styled/StyledCode.ts

+    default:
+      fgColorArgs.variable = 'foreground.default';
+      bgColorArgs.hue = 'neutralHue';
+      bgColorArgs.dark = { shade: 900 };
+      bgColorArgs.light = { shade: 200 };
+      break;


Unsupported hues default to grey. If a hue isn't recognized, getColor throws.

This move unintentionally breaks the Code hue API. The expectation is that hue can receive any value that getColor's hue argument can receive. So that includes all Garden hue values (lime, fuschia, etc), hex, etc. So a few things need to happen here:

continue to give special treatment to Garden's primary colors

get feedback from design on what shades to fallback to (perhaps dark:900 & light: 200)

allow getColor to do it's thing – including throwing an error if someone provides a bogus value

Please see StyledTag for comparison.

StyledCode is "loosely" typed with hue?: string

react-components/packages/typography/src/styled/StyledCode.ts

Lines 31 to 33 in cffdb06

interface IStyledCodeProps extends Omit<IStyledFontProps, 'size'> {

hue?: string;

size?: ICodeProps['size'];

In our case, only Code uses this component and restricts possible hue values to only grey, red, green, and yellow.

react-components/packages/typography/src/types/index.ts

Line 10 in 327ea52

export const HUE = ['grey', 'red', 'green', 'yellow'] as const;

react-components/packages/typography/src/types/index.ts

Lines 84 to 89 in 327ea52

export interface ICodeProps extends HTMLAttributes<HTMLElement> {

/** Applies color to the background and the text */

hue?: (typeof HUE)[number];

/** Adjusts the font size. By default font size is inherited from the surrounding text. */

size?: (typeof INHERIT_SIZE)[number];

}

Therefore, I don't think it's a breaking change, but rather a call to improve clarity by redefining the hue type in StyledCode to match its real-life usage. I must emphasize this suggestion only make sense because StyledCode is exclusively used by Code.

interface IStyledCodeProps extends Omit<IStyledFontProps, 'size'> { hue?: (typeof HUE)[number]; size?: ICodeProps['size']; }

Toughts?

Ah, interesting – I was looking at the wrong (Styled) API and testing on v8 with usage like <Code hue="fuschia">. In this case, restricting the hue color values makes sense. I still think allowing getColor to throw on invalid usage – as it now does in other v9 components – is appropriate and better than protecting the consumer from themselves.

ze-flo · 2024-06-01T00:29:45Z

packages/typography/src/styled/StyledFont.tsx

  }

-  if (props.hue) {
-    const shade = props.hue === 'yellow' ? 700 : 600;


No longer needed since the default shade for all hues is now 700 in light mode.

ze-flo · 2024-06-01T00:30:21Z

packages/typography/src/styled/StyledFont.tsx

  let fontWeight;
  let lineHeight;
-  let color;
+  let color = getColor({ theme, variable: 'foreground.default' });


Sets default color correctly based on mode.

@jzempel To maintain the current behavior, do we need to keep color as undefined? If so, the color would be inherited from the parent container. It will also be the user's responsibility to set the correct color based on the mode they're in.

Yes, the current API structure needs to be kept (aka undefined)

packages/typography/src/styled/StyledEllipsis.ts

packages/typography/src/styled/StyledParagraph.ts

ze-flo · 2024-06-01T00:33:04Z

packages/typography/src/elements/span/Span.spec.tsx

+
+      expect(container.firstChild).toHaveStyleRule(
+        'color',
+        (PALETTE as any)[hue][mode === 'light' ? 700 : 500]


Follows getColor's logic:

react-components/packages/theming/src/utils/getColor.ts

Line 34 in fcf7892

_shade = scheme === 'dark' ? 500 : 700;

ze-flo · 2024-06-01T00:34:25Z

packages/typography/src/elements/span/Span.spec.tsx

+      );
    });

-    it('handles yellow hue with specialized shading', () => {


Not needed since every hue's shade is 700 in light mode.

ze-flo · 2024-06-01T00:36:28Z

packages/typography/src/styled/StyledFont.tsx

-
-    color = getColorV8(props.hue, shade, props.theme);
+  if (hue) {
+    color = getColor({ hue, theme });


Using default shades:

react-components/packages/theming/src/utils/getColor.ts

Line 34 in fcf7892

_shade = scheme === 'dark' ? 500 : 700;

… ze-flo/typography-recolor

ze-flo · 2024-06-03T23:59:21Z

packages/typography/src/elements/Code.spec.tsx

-      expect(container.firstChild).toHaveStyleRule('background-color', PALETTE_V8.yellow[200]);
-    });
-  });
 });


Moved specs to StyledCode.spec.tsx.

This is fine for now. However, the prevailing recent thinking is to test at the element component level rather than at the view component. This way we're penetrating all points of interest. See the newer Combobox and Menu components in the dropdowns package for comparison.

Thank you for clarifying. I wasn't sure about that. I'll move the specs back to this file instead.

coveralls · 2024-06-04T00:12:58Z

coverage: 96.094% (+0.01%) from 96.084%
when pulling 4b3c78e on ze-flo/typography-recolor
into cffdb06 on next.

lucijanblagonic · 2024-06-04T09:10:06Z

Everything looks good. Why was the Code and CodeBlock excluded?

jzempel · 2024-06-04T15:12:42Z

packages/typography/src/elements/Code.spec.tsx

-      expect(container.firstChild).toHaveStyleRule('background-color', PALETTE_V8.yellow[200]);
-    });
-  });
 });


This is fine for now. However, the prevailing recent thinking is to test at the element component level rather than at the view component. This way we're penetrating all points of interest. See the newer Combobox and Menu components in the dropdowns package for comparison.

jzempel · 2024-06-04T15:21:20Z

packages/typography/src/styled/StyledCode.ts

+      bgColorArgs.hue = 'neutralHue';
+      bgColorArgs.dark = { shade: 900 };
+      bgColorArgs.light = { shade: 200 };


Suggested change

bgColorArgs.hue = 'neutralHue';

bgColorArgs.dark = { shade: 900 };

bgColorArgs.light = { shade: 200 };

bgColorArgs.variable = 'background.subtle';

bgColorArgs.dark = { offset: -100 };

bgColorArgs.light = { offset: 100 };

Always better to tie into a variable whenever possible.

Design opted for a custom component variable for this one:

WIP_code-bg-default #E8EAEC

I chose to explicitly build the color to reflect their choice and highlight the lack for variable for this hue / shade combo.

I think modifying an existing variable can lead to side-effects and increased maintenance overhead.

For ex, if Design were to change the shade of background.subtle, ENG would have to review all uses of this variable to ensure the resulting shade from offsets is still the desired one.

We already have guidance from design to use getColor's offset feature when possible. They wouldn't be using WIP variables if Figma provided similar functionality. Let's double check the semantics with them. As far as any variables change goes, we would have to review all uses – so I don't think that's a major issue here.

Following our convo, @jzempel and I decided it was best to leave as-is to better handle future themes.

Let's be sure to double-check intent with design.

jzempel · 2024-06-04T17:47:53Z

packages/typography/src/styled/StyledCode.ts

+    default:
+      fgColorArgs.variable = 'foreground.default';
+      bgColorArgs.hue = 'neutralHue';
+      bgColorArgs.dark = { shade: 900 };
+      bgColorArgs.light = { shade: 200 };
+      break;


This move unintentionally breaks the Code hue API. The expectation is that hue can receive any value that getColor's hue argument can receive. So that includes all Garden hue values (lime, fuschia, etc), hex, etc. So a few things need to happen here:

continue to give special treatment to Garden's primary colors

get feedback from design on what shades to fallback to (perhaps dark:900 & light: 200)

allow getColor to do it's thing – including throwing an error if someone provides a bogus value

Please see StyledTag for comparison.

packages/typography/src/styled/StyledEllipsis.ts

packages/typography/src/styled/StyledParagraph.ts

ze-flo · 2024-06-04T22:00:41Z

packages/typography/src/styled/StyledCode.ts


 interface IStyledCodeProps extends Omit<IStyledFontProps, 'size'> {
-  hue?: string;
+  hue?: ICodeProps['hue'];


hue?: string; was causing confusion. So I refined the type to emphasize that Code, StyledCode's only consumer, strictly supports grey, red, green, yellow, and undefined.

react-components/packages/typography/src/types/index.ts

Line 10 in 327ea52

export const HUE = ['grey', 'red', 'green', 'yellow'] as const;

Passing an unsupported hue would trigger a TS error or an invalid PropType error at runtime.

ze-flo · 2024-06-04T22:02:15Z

packages/typography/src/styled/StyledCode.ts

+      bgColorArgs.hue = 'neutralHue';
+      bgColorArgs.dark = { shade: 900 };
+      bgColorArgs.light = { shade: 200 };


Following our convo, @jzempel and I decided it was best to leave as-is to better handle future themes.

ze-flo · 2024-06-04T22:05:32Z

packages/typography/src/styled/StyledCode.spec.tsx

@@ -1,77 +0,0 @@
-/**


Consolidated into Code.spec.tsx.

ze-flo · 2024-06-04T22:13:04Z

packages/typography/src/styled/StyledCode.ts

  'data-garden-version': PACKAGE_VERSION,
-  as: 'code'
+  as: 'code',
+  isMonospace: true


isMonospace: true is required by StyledFont to apply the correct styles.

Defining it as a defaultProp didn't make that obvious.

This pattern aligns with other components. For ex:

react-components/packages/buttons/src/styled/StyledAnchor.ts

Lines 18 to 25 in cffdb06

export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => ({

'data-garden-id': COMPONENT_ID,

'data-garden-version': PACKAGE_VERSION,

as: 'a',

dir: props.theme.rtl ? 'rtl' : undefined,

isLink: true,

type: undefined

}))`

steue

this is looking good! have some clarifications on bg treatment for code components. design did some optical adjustments to the BG and shifted the treatments up 1 step. this would only affect success, danger, and warning. default looks good.

other than that, other elements LGTM!
cc: @lucijanblagonic

jzempel

nice – hits API expectations

sieeeeeenna · 2024-06-07T00:42:21Z

Looks good to me! These colors are a nice improvement!

steue

looks awesome thank you!

ze-flo added 2 commits May 31, 2024 12:34

chore: add reminder to remove `getRenderFn

8bac70c

feat(typography): new light/dark mode colors

66c16cc

ze-flo added the PR: New Feature 🚀 label Jun 1, 2024

ze-flo commented Jun 1, 2024

View reviewed changes

ze-flo requested review from lucijanblagonic and steue June 1, 2024 00:38

ze-flo marked this pull request as ready for review June 1, 2024 00:38

ze-flo requested a review from a team as a code owner June 1, 2024 00:38

ze-flo added 2 commits June 3, 2024 13:54

Merge branch 'next' of github.com:zendeskgarden/react-components into…

8a6f3a0

… ze-flo/typography-recolor

refactor: use getRenderFn from test utils

53035b5

ze-flo commented Jun 3, 2024

View reviewed changes

zendesk-garden deployed to staging June 4, 2024 00:13 View deployment

lucijanblagonic approved these changes Jun 4, 2024

View reviewed changes

jzempel requested changes Jun 4, 2024

View reviewed changes

ze-flo added 3 commits June 4, 2024 11:12

revert(Paragraph, Ellipsis): continue inheriting colors

888b20e

chore: PR feedback

4bcce5c

docs(storybook): set preview color to foreground.default

6af55b9

zendesk-garden deployed to staging June 4, 2024 22:10 View deployment

ze-flo commented Jun 4, 2024

View reviewed changes

steue suggested changes Jun 4, 2024

View reviewed changes

ze-flo added 2 commits June 5, 2024 09:42

fix(Code): refine colors

a3d6d05

revert(StyledFont): restore default color to undefined

4b3c78e

zendesk-garden deployed to staging June 5, 2024 20:13 View deployment

jzempel approved these changes Jun 6, 2024

View reviewed changes

steue approved these changes Jun 7, 2024

View reviewed changes

ze-flo merged commit 7a3ba84 into next Jun 7, 2024

ze-flo deleted the ze-flo/typography-recolor branch June 7, 2024 15:56

	interface IStyledCodeProps extends Omit<IStyledFontProps, 'size'> {
	hue?: string;
	size?: ICodeProps['size'];

	export interface ICodeProps extends HTMLAttributes<HTMLElement> {
	/** Applies color to the background and the text */
	hue?: (typeof HUE)[number];
	/** Adjusts the font size. By default font size is inherited from the surrounding text. */
	size?: (typeof INHERIT_SIZE)[number];
	}

	export const StyledAnchor = styled(StyledButton).attrs<IAnchorProps>(props => ({
	'data-garden-id': COMPONENT_ID,
	'data-garden-version': PACKAGE_VERSION,
	as: 'a',
	dir: props.theme.rtl ? 'rtl' : undefined,
	isLink: true,
	type: undefined
	}))`

feat(typography): new light/dark mode colors (excluding CodeBlock) #1820

feat(typography): new light/dark mode colors (excluding CodeBlock) #1820

Uh oh!

Conversation

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

Uh oh!

Description

Detail

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ze-flo Jun 4, 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!

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!

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!

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

Uh oh!

Uh oh!

lucijanblagonic commented Jun 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jzempel Jun 4, 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!

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!

Choose a reason for hiding this comment

Uh oh!

steue left a comment • 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!

jzempel left a comment

ze-flo commented Jun 1, 2024 •

edited

Loading

ze-flo Jun 4, 2024 •

edited

Loading

coveralls commented Jun 4, 2024 •

edited

Loading

jzempel Jun 4, 2024 •

edited

Loading

steue left a comment •

edited

Loading