Skip to content

feat: Dynamic theme selection#1511

Closed
bmingles wants to merge 2 commits intodeephaven:mainfrom
bmingles:1504-theme-loader-css
Closed

feat: Dynamic theme selection#1511
bmingles wants to merge 2 commits intodeephaven:mainfrom
bmingles:1504-theme-loader-css

Conversation

@bmingles
Copy link
Copy Markdown
Contributor

@bmingles bmingles commented Sep 13, 2023

Draft PR showing dynamic theme selection.

  • Theme key for selected theme is stored in local storage
  • Loads default + custom theme files from public folder (default_theme.css & custom_theme.css respectively)
  • Custom theme files can specify --dh-base-theme to inherit from a default theme
  • Loader assigns appropriate css class for default + custom theme to body element
  • IrisGrid themes replace any css variable references via getComputedStyles + getPropertyValue

This includes a few minimal changes to BaseStyleSheet.scss, goldenlayout-dark-theme.scss, and IrisGridTheme.ts to prove they can consume the theme variables.

NOTES:

  • We will need to determine custom themes dynamically likely from a manifest. This is not implemented yet
  • This does not yet include an example of Spectrum theming, but we should just be able to map their custom css props to our dh-xxxx ones.

To see it work, you can set the theme key in local storage in the console and then refresh the page. You should see different css classes get attached to the body based on the theme key:

localStorage.setItem('deephaven.themeKey', '') // should load default dark theme

localStorage.setItem('deephaven.themeKey', 'custom-aaa') // should load custom-aaa + default-dark theme

localStorage.setItem('deephaven.themeKey', 'custom-bbb') // should load custom-bbb + default-light theme

localStorage.setItem('deephaven.themeKey', 'custom-ccc') // should load custom-ccc + default-dark theme

#1504

result[key] =
typeof value === 'string'
? value.replace(CSS_PROPERTY_NAME_REGEX, (_match, propertyName) =>
getComputedStyle(targetElement ?? document.body).getPropertyValue(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

check my draft PR for converting rgba to hex with alpha, as that's needed for monaco.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah yes. I saw that. This is mostly just to show case "where" we would hook the replacement into Iris, but yeah needs to be built out more.


logInit();

initializeTheme();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We might want to inline and call this function in a script tag in the actual html , that way there's no flash of the wrong theme before react loads/initializes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm fine with that. Are you actually seeing a flash?

<link rel="icon" href="%VITE_FAVICON%" />
<title>Deephaven</title>
<link rel="stylesheet" href="/default_theme.css" />
<link rel="stylesheet" href="/custom_theme.css" />
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this more of a POC or intended implementation of how custom themes would be loaded?

It's probably preferable that they are split into separate files, and then loaded dynamically, such that theme files can be created and shared individually.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I imagine the default themes would actually live in the codebase not in public. Those could be split up however.

For the custom ones, it's easiest to just have 1 file and statically load it. I could see the benefit of splitting them up, would just have to see how hard it is to determine the full list before our app code has actually loaded

@bmingles
Copy link
Copy Markdown
Contributor Author

Closing this in favor of #1524

1 similar comment
@bmingles
Copy link
Copy Markdown
Contributor Author

Closing this in favor of #1524

@bmingles bmingles closed this Sep 20, 2023
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 20, 2023
@bmingles bmingles deleted the 1504-theme-loader-css branch September 25, 2023 15:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants