Skip to content

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

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.

Sign up for GitHub

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

Merged
ze-flo merged 9 commits into from
Jun 7, 2024
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8bac70c
chore: add reminder to remove `getRenderFn
ze-flo May 31, 2024
66c16cc
feat(typography): new light/dark mode colors
ze-flo May 31, 2024
8a6f3a0
Merge branch 'next' of github.com:zendeskgarden/react-components into…
ze-flo Jun 3, 2024
53035b5
refactor: use getRenderFn from test utils
ze-flo Jun 3, 2024
888b20e
revert(Paragraph, Ellipsis): continue inheriting colors
ze-flo Jun 4, 2024
4bcce5c
chore: PR feedback
ze-flo Jun 4, 2024
6af55b9
docs(storybook): set preview color to `foreground.default`
ze-flo Jun 4, 2024
a3d6d05
fix(Code): refine colors
ze-flo Jun 5, 2024
4b3c78e
revert(StyledFont): restore default color to undefined
ze-flo Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .storybook/preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ const GlobalPreviewStyling = createGlobalStyle`
background-color: ${p => getColor({ theme: p.theme, variable: 'background.default' })};
/* stylelint-disable-next-line declaration-no-important */
padding: 0 !important;
color: ${p => getColor({ theme: p.theme, variable: 'foreground.default' })};
font-family: ${p => p.theme.fonts.system};
}
`;
Expand Down
96 changes: 73 additions & 23 deletions packages/typography/src/elements/Code.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,18 @@
*/

import React from 'react';
import { render, renderRtl } from 'garden-test-utils';
import { PALETTE_V8 } from '@zendeskgarden/react-theming';
import { getRenderFn, render, renderRtl } from 'garden-test-utils';
import { PALETTE } from '@zendeskgarden/react-theming';
import { Code } from './Code';
import { ICodeProps } from 'packages/typography/src/types';

describe('Code', () => {
it('renders the expected element', () => {
const { container } = render(<Code />);

expect(container.firstChild!.nodeName).toBe('CODE');
});

it('applies correct styling with RTL locale', () => {
const { container } = renderRtl(<Code />);

Expand All @@ -26,7 +33,23 @@ describe('Code', () => {
);
});

it('renders anchor-inherited color', () => {
const { getByTestId } = render(
<a href="test">
<Code data-test-id="test" />
</a>
);

expect(getByTestId('test')).toHaveStyleRule('color', 'inherit', { modifier: 'a &' });
});

describe('size', () => {
it('renders inherited size', () => {
const { container } = render(<Code />);

expect(container.firstChild).not.toHaveStyleRule('font-size', 'inherit');
});

it('renders small styling if provided', () => {
const { container } = render(<Code size="small" />);

Expand All @@ -47,28 +70,55 @@ describe('Code', () => {
});

describe('hue', () => {
it('renders grey hue if provided', () => {
const { container } = render(<Code hue="grey" />);

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

it('renders green hue if provided', () => {
const { container } = render(<Code hue="green" />);

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

it('renders red hue if provided', () => {
const { container } = render(<Code hue="red" />);

expect(container.firstChild).toHaveStyleRule('background-color', PALETTE_V8.red[200]);
it.each<
[
ICodeProps['hue'],
'dark' | 'light',
{
color: string;
bgColor: string;
}
]
>([
// light mode
['grey', 'light', { color: PALETTE.grey[900], bgColor: PALETTE.grey[200] }],
['green', 'light', { color: PALETTE.green[900], bgColor: PALETTE.green[100] }],
['red', 'light', { color: PALETTE.red[900], bgColor: PALETTE.red[100] }],
['yellow', 'light', { color: PALETTE.yellow[900], bgColor: PALETTE.yellow[100] }],
// dark mode
['grey', 'dark', { color: PALETTE.grey[300], bgColor: PALETTE.grey[900] }],
['green', 'dark', { color: PALETTE.green[300], bgColor: PALETTE.green[1000] }],
['red', 'dark', { color: PALETTE.red[300], bgColor: PALETTE.red[1000] }],
['yellow', 'dark', { color: PALETTE.yellow[300], bgColor: PALETTE.yellow[1000] }]
])('renders with a "%s" hue in "%s" mode', (hue, mode, { color, bgColor }) => {
const { container } = getRenderFn(mode)(<Code hue={hue} />);

expect(container.firstChild).toHaveStyleRule('color', color);
expect(container.firstChild).toHaveStyleRule('background-color', bgColor);
});

it('renders yellow hue if provided', () => {
const { container } = render(<Code hue="yellow" />);

expect(container.firstChild).toHaveStyleRule('background-color', PALETTE_V8.yellow[200]);
});
it.each<
[
string | undefined,
'dark' | 'light',
{
color: string;
bgColor: string;
}
]
>([
['azure', 'light', { color: PALETTE.grey[900], bgColor: PALETTE.grey[200] }],
[undefined, 'light', { color: PALETTE.grey[900], bgColor: PALETTE.grey[200] }],
['azure', 'dark', { color: PALETTE.grey[300], bgColor: PALETTE.grey[900] }],
[undefined, 'dark', { color: PALETTE.grey[300], bgColor: PALETTE.grey[900] }]
])(
'outputs the grey hue color combination for an unsupported "%s" hue in "%s" mode',
(hue, mode, { color, bgColor }) => {
const { container } = getRenderFn(mode)(<Code hue={hue as any} />);

expect(container.firstChild).toHaveStyleRule('color', color);
expect(container.firstChild).toHaveStyleRule('background-color', bgColor);
}
);
});
});
Copy link
Contributor Author

@ze-flo ze-flo Jun 3, 2024

Choose a reason for hiding this comment

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

Moved specs to StyledCode.spec.tsx.

Copy link
Member

@jzempel jzempel Jun 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ze-flo ze-flo Jun 4, 2024

Choose a reason for hiding this comment

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

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

68 changes: 40 additions & 28 deletions packages/typography/src/elements/span/Span.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@
*/

import React from 'react';
import { DEFAULT_THEME, PALETTE_V8 } from '@zendeskgarden/react-theming';
import { render, renderRtl } from 'garden-test-utils';
import { DEFAULT_THEME, PALETTE } from '@zendeskgarden/react-theming';
import { getRenderFn, render, renderRtl } from 'garden-test-utils';
import { Span } from './Span';
import TestIcon from '@zendeskgarden/svg-icons/src/16/gear-stroke.svg';

Expand Down Expand Up @@ -47,35 +47,47 @@ describe('Span', () => {
});

describe('hue', () => {
it('renders the hue provided', () => {
[
'grey',
'blue',
'kale',
'red',
'green',
'fuschia',
'pink',
'crimson',
'orange',
'lemon',
'lime',
'mint',
'teal',
'azure',
'royal',
'purple'
].forEach(color => {
const { container } = render(<Span hue={color as any} />);

expect(container.firstChild).toHaveStyleRule('color', (PALETTE_V8 as any)[color][600]);
});
const cases = [
'grey',
'blue',
'kale',
'red',
'green',
'fuschia',
'pink',
'crimson',
'orange',
'lemon',
'lime',
'mint',
'teal',
'azure',
'royal',
'purple',
'yellow'
].reduce<[string, 'light' | 'dark'][]>((arr, hue) => {
arr.push([hue, 'light']);
arr.push([hue, 'dark']);

return arr;
}, []);

it.each(cases)('renders with a "%s" hue in "%s" mode', (hue, mode) => {
const { container } = getRenderFn(mode)(<Span hue={hue} />);

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

@ze-flo ze-flo Jun 1, 2024

Choose a reason for hiding this comment

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

Follows getColor's logic:

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

Line 34 in fcf7892

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

);
});

it('handles yellow hue with specialized shading', () => {
Copy link
Contributor Author

@ze-flo ze-flo Jun 1, 2024

Choose a reason for hiding this comment

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

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

const { container } = render(<Span hue="yellow" />);
it.each<['light' | 'dark', string]>([
['light', PALETTE.grey[900]],
['dark', PALETTE.grey[300]]
])('renders with a default hue in "%s" mode', (mode, color) => {
const { container } = getRenderFn(mode)(<Span />);

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

Expand Down
4 changes: 2 additions & 2 deletions packages/typography/src/styled/StyledBlockquote.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/

import styled from 'styled-components';
import { DEFAULT_THEME, getColorV8, retrieveComponentStyles } from '@zendeskgarden/react-theming';
import { DEFAULT_THEME, getColor, retrieveComponentStyles } from '@zendeskgarden/react-theming';
import { IBlockquoteProps } from '../types';
import { THEME_SIZES } from './StyledFont';

Expand All @@ -20,7 +20,7 @@ export const StyledBlockquote = styled.blockquote.attrs({
/* stylelint-disable property-no-unknown */
border-${props => (props.theme.rtl ? 'right' : 'left')}: ${props =>
props.theme.shadowWidths.sm} solid;
border-color: ${props => getColorV8('neutralHue', 400, props.theme)};
border-color: ${props => getColor({ theme: props.theme, variable: 'border.default' })};
padding: 0;
padding-${props => (props.theme.rtl ? 'right' : 'left')}: ${props =>
props.theme.space.base * 4}px;
Expand Down
77 changes: 0 additions & 77 deletions packages/typography/src/styled/StyledCode.spec.tsx
View file
Open in desktop

This file was deleted.

43 changes: 33 additions & 10 deletions packages/typography/src/styled/StyledCode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,40 @@
*/

import styled, { css, DefaultTheme, ThemeProps } from 'styled-components';
import { DEFAULT_THEME, getColorV8, retrieveComponentStyles } from '@zendeskgarden/react-theming';
import { DEFAULT_THEME, getColor, retrieveComponentStyles } from '@zendeskgarden/react-theming';
import { StyledFont, IStyledFontProps } from './StyledFont';
import { ICodeProps } from '../types';

const COMPONENT_ID = 'typography.code';

const colorStyles = (props: IStyledCodeProps & ThemeProps<DefaultTheme>) => {
const hue = props.hue || 'neutralHue';
const backgroundColor = getColorV8(hue, 200, props.theme);
const shade = hue === 'yellow' ? 800 : 700;
const foregroundColor = getColorV8(hue, shade, props.theme);
const colorStyles = ({ hue, theme }: IStyledCodeProps & ThemeProps<DefaultTheme>) => {
const bgColorArgs: Parameters<typeof getColor>[0] = { theme };
const fgColorArgs: Parameters<typeof getColor>[0] = { theme };

switch (hue) {
case 'green':
bgColorArgs.variable = 'background.success';
fgColorArgs.variable = 'foreground.successEmphasis';
break;
case 'red':
bgColorArgs.variable = 'background.danger';
fgColorArgs.variable = 'foreground.dangerEmphasis';
break;
case 'yellow':
bgColorArgs.variable = 'background.warning';
fgColorArgs.variable = 'foreground.warningEmphasis';
break;
// includes grey
default:
fgColorArgs.variable = 'foreground.default';
bgColorArgs.hue = 'neutralHue';
bgColorArgs.dark = { shade: 900 };
bgColorArgs.light = { shade: 200 };
Copy link
Member

@jzempel jzempel Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ze-flo ze-flo Jun 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@jzempel jzempel Jun 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ze-flo ze-flo Jun 4, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@jzempel jzempel Jun 5, 2024

Choose a reason for hiding this comment

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

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

break;
Copy link
Contributor Author

@ze-flo ze-flo Jun 1, 2024

Choose a reason for hiding this comment

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

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

Copy link
Member

@jzempel jzempel Jun 4, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ze-flo ze-flo Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@jzempel jzempel Jun 4, 2024

Choose a reason for hiding this comment

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

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.

}

const backgroundColor = getColor(bgColorArgs);
const foregroundColor = getColor(fgColorArgs);

return css`
background-color: ${backgroundColor};
Expand All @@ -29,14 +52,15 @@ const colorStyles = (props: IStyledCodeProps & ThemeProps<DefaultTheme>) => {
};

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

@ze-flo ze-flo Jun 4, 2024

Choose a reason for hiding this comment

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

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.

size?: ICodeProps['size'];
}

export const StyledCode = styled(StyledFont as 'code').attrs({
'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION,
as: 'code'
as: 'code',
isMonospace: true
Copy link
Contributor Author

@ze-flo ze-flo Jun 4, 2024

Choose a reason for hiding this comment

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

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
}))`

})<IStyledCodeProps>`
border-radius: ${props => props.theme.borderRadii.sm};
padding: 1.5px;
Expand All @@ -48,7 +72,6 @@ export const StyledCode = styled(StyledFont as 'code').attrs({

StyledCode.defaultProps = {
theme: DEFAULT_THEME,
isMonospace: true,
hue: 'neutralHue',
hue: 'grey',
size: 'inherit'
};
Loading