[ui] Use CSS vars for theming #21200

hellendag · 2024-04-12T22:33:36Z

Summary & Motivation

Use CSS vars instead of JS strings to set themed values.

The main benefit of this is that it will no longer require reloading the page to see the theme update: when you change the theme in User Settings, it will update immediately. Additionally, if the user's OS is set to change from light to dark (and vice versa) based on time of day, and they are using the "System" theme, the theme change should now occur automatically, without a browser reload.

I have addressed a couple of items with inline comments.

Screen.Recording.2024-04-15.at.13.14.03.mov

How I Tested These Changes

Load app, change theme. Verify that it updates right away. Click around the app, verify that everything looks correct.

hellendag · 2024-04-12T22:33:49Z

[ui] Use CSS vars for theming #21200 👈
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @hellendag and the rest of your teammates on Graphite

github-actions · 2024-04-12T22:36:28Z

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-pn0juh6sz-elementl.vercel.app
https://dish-theme-vars.components-storybook.dagster-docs.io

Built with commit 32d9686.
This pull request is being automatically deployed with vercel-action

github-actions · 2024-04-12T22:36:57Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-nanzb71j0-elementl.vercel.app
https://dish-theme-vars.core-storybook.dagster-docs.io

Built with commit 32d9686.
This pull request is being automatically deployed with vercel-action

hellendag · 2024-04-15T19:06:21Z

js_modules/dagster-ui/packages/ui-core/src/app/AppProvider.tsx

-        <GlobalPopoverStyle />
-        <GlobalDialogStyle />
-        <GlobalCustomAlertPortalStyle />
-        <GlobalSuggestStyle />


Moved all of this out to StyleProvider.

hellendag · 2024-04-15T19:08:56Z

js_modules/dagster-ui/packages/ui-core/src/graph/SVGViewport.tsx

@@ -633,7 +633,7 @@ const SVGViewportStyles: React.CSSProperties = {
  overflow: 'hidden',
  userSelect: 'none',
  outline: 'none',
-  background: `url("data:image/svg+xml;utf8,<svg width='30px' height='30px' viewBox='0 0 80 80' xmlns='http://www.w3.org/2000/svg'><circle fill='${Colors.lineageDots()}' cx='5' cy='5' r='5' /></svg>") repeat`,
+  background: `url("data:image/svg+xml;utf8,<svg width='30px' height='30px' viewBox='0 0 80 80' xmlns='http://www.w3.org/2000/svg'><circle fill='rgba(103, 116, 138, 0.20)' cx='5' cy='5' r='5' /></svg>") repeat`,


Using a CSS var on the inline SVG doesn't work, and it seemed like overkill to expose the rgba values in JS. It's simplest to just put an rgba value here, and this color looks fine in both modes.

thats a bummer, hopefully we don't run into more cases of this.

I didn't see any others, but yeah, kind of sucks. We might just have to take a different approach to rendering this pattern if it does need theming after all.

salazarm · 2024-04-15T19:50:43Z

js_modules/dagster-ui/packages/ui-components/src/theme/GlobalThemeStyle.tsx

+const ThemeRoot = createGlobalStyle`
+  @media (prefers-color-scheme: light) {
+    :root, .themeSystem {
+      ${lightThemeColors}
+    }
+  }
+
+  @media (prefers-color-scheme: dark) {
+    :root, .themeSystem {
+      ${darkThemeColors}
+    }
+  }
+
+  .themeLight {
+    ${lightThemeColors}
+  }
+
+  .themeDark {
+    ${darkThemeColors}
+  }
+`;


A shame this will end up a giant string including the same vars twice.
Style queries could help here https://developer.chrome.com/docs/css-ui/style-queries but the support is limited.

js_modules/dagster-ui/packages/ui-core/src/app/GlobalStyleProvider.tsx

salazarm · 2024-04-15T19:53:44Z

js_modules/dagster-ui/packages/ui-core/src/graph/SVGViewport.tsx

@@ -633,7 +633,7 @@ const SVGViewportStyles: React.CSSProperties = {
  overflow: 'hidden',
  userSelect: 'none',
  outline: 'none',
-  background: `url("data:image/svg+xml;utf8,<svg width='30px' height='30px' viewBox='0 0 80 80' xmlns='http://www.w3.org/2000/svg'><circle fill='${Colors.lineageDots()}' cx='5' cy='5' r='5' /></svg>") repeat`,
+  background: `url("data:image/svg+xml;utf8,<svg width='30px' height='30px' viewBox='0 0 80 80' xmlns='http://www.w3.org/2000/svg'><circle fill='rgba(103, 116, 138, 0.20)' cx='5' cy='5' r='5' /></svg>") repeat`,


thats a bummer, hopefully we don't run into more cases of this.

hellendag · 2024-04-15T19:54:35Z

Pushed up a change to enable theme switching within Storybook.

salazarm · 2024-04-15T20:06:40Z

js_modules/dagster-ui/packages/ui-components/src/components/__stories__/BaseButton.stories.tsx

@@ -31,35 +32,35 @@ export const Default = () => {
 export const Fill = () => {
  return (
    <Group direction="column" spacing={8}>
-      <BaseButton label="Button" fillColor={CoreColors.Gray900} textColor={CoreColors.White} />
+      <BaseButton label="Button" fillColor={Colors.accentGray()} textColor={Colors.alwaysWhite()} />


why is this alwaysWhite?

We're currently using white in all cases for the text on toasters and filled buttons, regardless of theme, but we were previously using CoreColors.White directly to accomplish this. I added alwaysWhite() within this PR to make it a "themable" color function like the others, though we could come up with a more appropriate semantic name for it.

bengotow

Wow this is so cool. Using variables to do this is much more elegant than what I thought this would require... and it's doubly cool that this doesn't block us from imperatively using the colors in random places like style values.

Only two things I can think to check

the "Download SVG" button in the asset graph should copy the CSS containing the variable declarations into the SVG, but I'm not 100% sure.
The library we use for asset "plots" sometimes uses canvas I think? I imagine the browser still resolves the variables in the canvas context but maybe worth opening that page to verify 👀

bengotow · 2024-04-16T00:02:35Z

js_modules/dagster-ui/packages/ui-components/src/components/Button.tsx

@@ -160,7 +159,7 @@ export const intentToTextAndIconColor = (intent: BlueprintIntent) => {
  if (intent === 'primary') {
    return Colors.accentReversed();
  }
-  return CoreColors.White;
+  return Colors.alwaysWhite();


I like this naming, was actually wondering just the other day whether white and black were static. 🙌

hellendag · 2024-04-16T17:20:51Z

Thanks for calling those out, @bengotow! I've put fixes in places for both of those:

When generating the SVG for download, if the style value is a var, use the computed style instead (since we've already got it available).
For Chart.js, we (sadly) need the RGB values. So I created a hook (useRGBColorsForTheme) which uses the computed style on document.body to provide a mapping from var string to RGB value. It's an escape hatch that I hope we won't need to use often -- since it's pretty gross to compute back from the body style for this -- but it does the job.

[INTERNAL_BRANCH=dish/stripe-theming]

## Summary & Motivation Use CSS vars instead of JS strings to set themed values. The main benefit of this is that it will no longer require reloading the page to see the theme update: when you change the theme in User Settings, it will update immediately. Additionally, if the user's OS is set to change from light to dark (and vice versa) based on time of day, and they are using the "System" theme, the theme change should now occur automatically, without a browser reload. I have addressed a couple of items with inline comments. https://github.com/dagster-io/dagster/assets/2823852/8e51bbce-6675-470b-b052-db7c51773bc4 ## How I Tested These Changes Load app, change theme. Verify that it updates right away. Click around the app, verify that everything looks correct.

hellendag force-pushed the dish/theme-vars branch from 98a7251 to 36a0c31 Compare April 15, 2024 18:15

hellendag commented Apr 15, 2024

View reviewed changes

hellendag requested review from salazarm and bengotow April 15, 2024 19:12

hellendag marked this pull request as ready for review April 15, 2024 19:12

hellendag requested a review from braunjj April 15, 2024 19:12

salazarm reviewed Apr 15, 2024

View reviewed changes

hellendag force-pushed the dish/theme-vars branch from 36a0c31 to 1125219 Compare April 15, 2024 19:52

salazarm approved these changes Apr 15, 2024

View reviewed changes

salazarm reviewed Apr 15, 2024

View reviewed changes

hellendag force-pushed the dish/theme-vars branch 2 times, most recently from 073cdb5 to adf5737 Compare April 15, 2024 22:01

bengotow approved these changes Apr 16, 2024

View reviewed changes

hellendag force-pushed the dish/theme-vars branch 3 times, most recently from fc83974 to 6ad7ee8 Compare April 16, 2024 17:19

hellendag force-pushed the dish/theme-vars branch from 6ad7ee8 to 1873620 Compare April 16, 2024 18:35

[ui] Use CSS vars for theming

32d9686

[INTERNAL_BRANCH=dish/stripe-theming]

hellendag force-pushed the dish/theme-vars branch from 1873620 to 32d9686 Compare April 18, 2024 15:31

hellendag merged commit 64ba7b8 into master Apr 18, 2024
2 of 3 checks passed

hellendag deleted the dish/theme-vars branch April 18, 2024 16:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ui] Use CSS vars for theming #21200

[ui] Use CSS vars for theming #21200

hellendag commented Apr 12, 2024 •

edited

Loading

hellendag commented Apr 12, 2024

github-actions bot commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading

hellendag Apr 15, 2024

hellendag Apr 15, 2024

salazarm Apr 15, 2024

hellendag Apr 15, 2024

salazarm Apr 15, 2024

salazarm Apr 15, 2024

hellendag commented Apr 15, 2024

salazarm Apr 15, 2024

hellendag Apr 15, 2024

bengotow left a comment

bengotow Apr 16, 2024

hellendag commented Apr 16, 2024

[ui] Use CSS vars for theming #21200

[ui] Use CSS vars for theming #21200

Conversation

hellendag commented Apr 12, 2024 • edited Loading

Summary & Motivation

How I Tested These Changes

hellendag commented Apr 12, 2024

github-actions bot commented Apr 12, 2024 • edited Loading

github-actions bot commented Apr 12, 2024 • edited Loading

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

hellendag commented Apr 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bengotow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellendag commented Apr 16, 2024

hellendag commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading

github-actions bot commented Apr 12, 2024 •

edited

Loading