feat(Modal): new light/dark mode colors #1808

ze-flo · 2024-05-14T22:33:16Z

Description

Adds dark / light mode support to Modal

Detail

Adds dark / light mode support to Modal
Reuse IconButton to remove duplicated css for StyledClose button

Light

Dark

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

…styles

ze-flo · 2024-05-14T22:45:42Z

packages/modals/src/styled/StyledClose.spec.tsx

-    const { container } = render(<StyledClose />);
+    const { container } = render(
+      <StyledClose>
+        <XStrokeIcon />


StyledClose extends IconButton and now requires a child.

ze-flo · 2024-05-14T22:47:42Z

packages/modals/src/styled/StyledClose.ts

- * 1. Remove dotted outline from Firefox on focus.
- */
-export const StyledClose = styled.button.attrs({
+export const StyledClose = styled(IconButton).attrs({
  'data-garden-id': COMPONENT_ID,
  'data-garden-version': PACKAGE_VERSION
 })`


Keeping as is for now even though attributes set on StyledIconButton take precedence.

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

Lines 72 to 74 in 5282298

export const StyledIconButton = styled(StyledButton as 'button').attrs({

'data-garden-id': COMPONENT_ID,

'data-garden-version': PACKAGE_VERSION

Hey @ze-flo, I have a commit on the buttons PR that resolves the issue 🙌

packages/modals/src/styled/StyledClose.ts

jzempel

Let's remove all breaking change (title, description, tags) mention on this PR in favor of having that discussion in #1804 which will resolve the matter. Also, let's get #1807 merged before obtaining designer review here.

jzempel · 2024-05-16T14:39:07Z

packages/modals/package.json

    "dom-helpers": "^5.1.0",
    "prop-types": "^15.5.7",
    "react-merge-refs": "^2.0.0",
    "react-transition-group": "^4.4.2"
  },
  "peerDependencies": {
+    "@zendeskgarden/react-buttons": ">=9.0.0-next",


Should not be added as a peer dependency. Compare with https://github.com/zendeskgarden/react-components/blob/main/packages/grid/package.json

jzempel · 2024-05-16T14:40:59Z

packages/modals/src/styled/StyledBody.ts

  font-size: ${props => props.theme.fontSizes.md};
-  ${props => retrieveComponentStyles(COMPONENT_ID, props)};
+  ${props => retrieveComponentStyles(props['data-garden-id'], props)};


Let's revert to COMPONENT_ID and keep the current pattern consistent. Defer discussion to #1804

packages/modals/src/styled/StyledClose.ts

jzempel · 2024-05-16T14:49:43Z

packages/modals/src/styled/StyledHeader.ts

+  border-bottom: ${({ theme }) =>
+    `${theme.borders.sm} ${getColor({ theme, variable: 'border.subtle' })}`};


When there are > 1 color-related styles, let's consolidate into a colorStyles function as directed by the API docs (and dark mode RFC). The border-bottom: ${props => props.theme.borders.sm}; stays here, and border-bottom-color can move to the colorStyles function.

Per RFC, consider opportunities to also extract sizeStyles when the lift isn't too big (I would probably do that here, as it would alleviate a lot of the inline styling clunkiness in favor of variable declaration clarity)

jzempel · 2024-05-16T14:54:29Z

packages/modals/src/styled/StyledModal.ts

+    theme,
+    hue: 'neutralHue',
+    shade: 1200,
+    light: { transparency: 0.15 },


This transparency looks incorrect. Please consult design re: theme.opacity[200]

jzempel · 2024-05-16T14:56:48Z

packages/modals/src/styled/StyledModal.ts

+    shade: 1200,
+    light: { transparency: 0.15 },
+    dark: { transparency: theme.opacity['800'] }
+  });

  return shadows.lg(offsetY, blurRadius, color as string);


Suggested change

return shadows.lg(offsetY, blurRadius, color as string);

return shadows.lg(offsetY, blurRadius, color);

Should no longer need the cast with getColor.

jzempel · 2024-05-16T15:05:58Z

packages/modals/src/styled/StyledModal.ts

+  border: ${({ theme }) =>
+    `${theme.borders.sm} ${getColor({ theme, variable: 'border.default' })}`};
  border-radius: ${props => props.theme.borderRadii.md};
  box-shadow: ${boxShadow};
-  background-color: ${props => getColorV8('background', 600 /* default shade */, props.theme)};
+  background-color: ${({ theme }) => getColor({ theme, variable: 'background.raised' })};


ditto re: extracting colorStyles

geotrev · 2024-05-16T20:40:42Z

Do we want to toggle the system color-scheme: dark property so scrollbars look correct? https://stackoverflow.com/questions/65940522/how-do-i-switch-to-chromes-dark-scrollbar-like-github-does

ze-flo · 2024-05-16T21:08:51Z

Do we want to toggle the system color-scheme: dark property so scrollbars look correct? https://stackoverflow.com/questions/65940522/how-do-i-switch-to-chromes-dark-scrollbar-like-github-does

That's a great idea! 🔥

Should individual components (in this case Modal.Body) or ThemeProvider dynamically set the color-scheme: dark | light?

Setting it at the component level is more error-prone since we may forget to add the rule.

Leaving that responsibility to ThemeProvider, by creating a global CSS rule via createGlobalStyle, offers a great benefit: every component will automatically inherit the color-scheme related styles.

… ze-flo/modal-recolor

jzempel · 2024-05-16T21:56:25Z

Do we want to toggle the system color-scheme: dark property so scrollbars look correct? https://stackoverflow.com/questions/65940522/how-do-i-switch-to-chromes-dark-scrollbar-like-github-does

That's a great idea! 🔥

Should individual components (in this case Modal.Body) or ThemeProvider dynamically set the color-scheme: dark | light?

Setting it at the component level is more error-prone since we may forget to add the rule.

Leaving that responsibility to ThemeProvider, by creating a global CSS rule via createGlobalStyle, offers a great benefit: every component will automatically inherit the color-scheme related styles.

I was going to say no, since consumers can use components without a theming parent. But then they could never toggle dark mode 😅. So I think ThemeProvider is a solid choice that also benefits custom components rendered within the theme.

ze-flo · 2024-05-17T00:40:05Z

Do we want to toggle the system color-scheme: dark property so scrollbars look correct? https://stackoverflow.com/questions/65940522/how-do-i-switch-to-chromes-dark-scrollbar-like-github-does

That's a great idea! 🔥
Should individual components (in this case Modal.Body) or ThemeProvider dynamically set the color-scheme: dark | light?
Setting it at the component level is more error-prone since we may forget to add the rule.
Leaving that responsibility to ThemeProvider, by creating a global CSS rule via createGlobalStyle, offers a great benefit: every component will automatically inherit the color-scheme related styles.

I was going to say no, since consumers can use components without a theming parent. But then they could never toggle dark mode 😅. So I think ThemeProvider is a solid choice that also benefits custom components rendered within the theme.

@steue Modal is ready for your VQA. Don't worry about the intensely-bright scrollbar in dark mode. This issue isn't solely connected to this component and will be addressed a follow-up PR.

geotrev · 2024-05-17T14:45:55Z

packages/modals/src/styled/StyledClose.ts

-
-  & > svg {
-    vertical-align: middle;
-  }


geotrev

Very nice!

jzempel

[nit] consider setting up local variables in your colorStyles functions rather than inlining in the generated CSS. It benefits CSS readability and future debug maintenance.

jzempel · 2024-05-17T14:59:48Z

packages/modals/src/styled/StyledModal.ts

+  const color = getColor({
+    theme,
+    hue: 'neutralHue',
+    shade: 1200,
+    light: { transparency: theme.opacity[200] },
+    dark: { transparency: theme.opacity[800] }
+  });
+
+  return shadows.lg(offsetY, blurRadius, color);
+};


Consistent with other Styled components, it would be preferable to ditch the boxShadow function in favor of consolidating the box-shadow property declaration within colorStyles below. See https://github.com/zendeskgarden/react-components/blob/main/docs/api.md#rules ${colorStyles} section for details.

… ze-flo/modal-recolor

coveralls · 2024-05-17T21:10:32Z

coverage: 96.075% (-0.01%) from 96.089%
when pulling 239cff0 on ze-flo/modal-recolor
into 12c7917 on next.

jzempel · 2024-05-20T21:17:37Z

packages/modals/src/styled/StyledModal.ts

+  const offsetY = `${theme.space.base * 5}px`;
+  const blurRadius = `${theme.space.base * 7}px`;


The spread is different for dark mode. Please compare with in-progress branch:

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

Lines 67 to 71 in 9f82a02

box-shadow: ${theme.shadows.lg(

`${theme.space.base * (theme.colors.base === 'dark' ? 4 : 5)}px`,

`${theme.space.base * (theme.colors.base === 'dark' ? 5 : 6)}px`,

boxShadowColor

)};

Good catch! 💯 There is difference in Dark mode. I'll update.

~~Unrelated... but should we raise these 1px shadow spread differences with design? They seem questionable. 🤔~~

Never mind. It's a multiplier. 😅

Already did 😉; @lucijanblagonic confirmed the difference is intentional.

steue · 2024-05-20T23:08:02Z

took a peak and it looks gr8!! just to clarify, i am just reviewing Modal and not tooltip and drawer? if so, then LGTM! thanks @ze-flo

jzempel · 2024-05-21T12:58:34Z

packages/modals/src/styled/StyledModal.ts

+  ${colorStyles};
+


[nit] per https://github.com/zendeskgarden/react-components/blob/main/docs/api.md#rules, ${colorStyles}; should follow ${sizeStyles};.

ze-flo added 3 commits May 14, 2024 10:57

refactor(Modal): add darkmode support

ba4f0f5

test(Modal Drawer Header): update specs

fe00e13

refactor!(Modal Close): replace custom btn w IconButton to inherit …

c0f8bcd

…styles

ze-flo added the PR: Breaking Change 💥 label May 14, 2024

ze-flo commented May 14, 2024

View reviewed changes

ze-flo marked this pull request as ready for review May 14, 2024 22:56

ze-flo requested a review from a team as a code owner May 14, 2024 22:56

jzempel reviewed May 16, 2024

View reviewed changes

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

4b0116e

… ze-flo/modal-recolor

ze-flo added 2 commits May 16, 2024 14:28

refactor: separate colorStyles

6c715f4

chore: pr feedback

5427c0d

ze-flo added PR: New Feature 🚀 and removed PR: Breaking Change 💥 labels May 17, 2024

ze-flo changed the title ~~feat!(Modal): new light/dark mode colors~~ feat(Modal): new light/dark mode colors May 17, 2024

ze-flo requested a review from steue May 17, 2024 00:33

zendesk-garden deployed to staging May 17, 2024 00:48 View deployment

geotrev reviewed May 17, 2024

View reviewed changes

packages/modals/src/styled/StyledClose.ts

& > svg {

vertical-align: middle;

}

Copy link

Contributor

geotrev May 17, 2024

Choose a reason for hiding this comment

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

🧹

geotrev reacted with laugh emoji

geotrev approved these changes May 17, 2024

View reviewed changes

jzempel requested changes May 17, 2024

View reviewed changes

ze-flo mentioned this pull request May 17, 2024

fix(Buttons): correctly support overriding default data-garden-id attribute #1804

Merged

7 tasks

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

5ebd9e4

… ze-flo/modal-recolor

zendesk-garden deployed to staging May 17, 2024 21:11 View deployment

style: improve readability

714f8d7

zendesk-garden deployed to staging May 20, 2024 17:51 View deployment

jzempel requested changes May 20, 2024

View reviewed changes

fix(Modal): correct shadow values based on mode

5bea5b3

zendesk-garden deployed to staging May 20, 2024 22:22 View deployment

jzempel approved these changes May 21, 2024

View reviewed changes

style(Modal): fix css rules order

239cff0

zendesk-garden deployed to staging May 22, 2024 16:25 View deployment

ze-flo merged commit 2649a2a into next May 22, 2024
5 checks passed

ze-flo deleted the ze-flo/modal-recolor branch May 22, 2024 16:28

ze-flo mentioned this pull request Jun 18, 2024

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

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Modal): new light/dark mode colors #1808

feat(Modal): new light/dark mode colors #1808

ze-flo commented May 14, 2024 •

edited

Loading

ze-flo May 14, 2024

ze-flo May 14, 2024

jzempel May 15, 2024

jzempel left a comment

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

jzempel May 16, 2024

geotrev commented May 16, 2024

ze-flo commented May 16, 2024

jzempel commented May 16, 2024

ze-flo commented May 17, 2024

geotrev May 17, 2024

geotrev left a comment

jzempel left a comment

jzempel May 17, 2024

coveralls commented May 17, 2024 •

edited

Loading

jzempel May 20, 2024

ze-flo May 20, 2024 •

edited

Loading

jzempel May 20, 2024

steue commented May 20, 2024

jzempel May 21, 2024

	export const StyledIconButton = styled(StyledButton as 'button').attrs({
	'data-garden-id': COMPONENT_ID,
	'data-garden-version': PACKAGE_VERSION

		border-bottom: ${({ theme }) =>
		`${theme.borders.sm} ${getColor({ theme, variable: 'border.subtle' })}`};

	return shadows.lg(offsetY, blurRadius, color as string);
	return shadows.lg(offsetY, blurRadius, color);

		const offsetY = `${theme.space.base * 5}px`;
		const blurRadius = `${theme.space.base * 7}px`;

	box-shadow: ${theme.shadows.lg(
	`${theme.space.base * (theme.colors.base === 'dark' ? 4 : 5)}px`,
	`${theme.space.base * (theme.colors.base === 'dark' ? 5 : 6)}px`,
	boxShadowColor
	)};

feat(Modal): new light/dark mode colors #1808

feat(Modal): new light/dark mode colors #1808

Conversation

ze-flo commented May 14, 2024 • edited Loading

Description

Detail

Light

Dark

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzempel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geotrev commented May 16, 2024

ze-flo commented May 16, 2024

jzempel commented May 16, 2024

ze-flo commented May 17, 2024

Choose a reason for hiding this comment

geotrev left a comment

Choose a reason for hiding this comment

jzempel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 17, 2024 • edited Loading

Choose a reason for hiding this comment

ze-flo May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steue commented May 20, 2024

Choose a reason for hiding this comment

ze-flo commented May 14, 2024 •

edited

Loading

coveralls commented May 17, 2024 •

edited

Loading

ze-flo May 20, 2024 •

edited

Loading