Skip to content
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.

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

Theming - merge default preload data into cached preload data #1695

Closed
bmingles opened this issue Dec 17, 2023 · 0 comments · Fixed by #1723
Closed

Theming - merge default preload data into cached preload data #1695

bmingles opened this issue Dec 17, 2023 · 0 comments · Fixed by #1723
Assignees
Labels
bug Something isn't working enhancement New feature or request
Milestone

Comments

@bmingles
Copy link
Contributor

bmingles commented Dec 17, 2023

We "preload" a few css theming variables that are needed before plugins have a chance to load. ThemeUtils.preloadTheme() first attempts to load the variables from the localStorage cache. If this fails, or if no cached variables exist, it falls back to calculatePreloadStyleContent() where default values for our variables are provided. This means that when variables exist in the cache, calculatePreloadStyleContent() won't be called before the initial page load. This is by design under normal circumstances, but in cases where we may update the list of preload variables, it means the user can experience a 1 time page load that is missing colors. This will resolve itself after first login but does introduce the possibility of a user experiencing missing colors on the login page immediately following a DHC update.

To see the issue, logout of DHE, run the following in dev tools, and refresh the page

localStorage.setItem('deephaven.themeCache', '{"themeKey":"default-dark","preloadStyleContent":":root{--dh-color-input-bg:#fff}"}')

Note that DHC is less impacted by this issue since plugins load before login, but a user may still se the loading spinner or background color be off on initial page load.

One fix that could mitigate this would be to always call calculatePreloadStyleContent() and then override variables that are in the cache. This would ensure we always get new variables. It would not handle a scenario where we change a default variable value, but that's probably ok since things will resolve after login.

@bmingles bmingles added bug Something isn't working enhancement New feature or request triage Issue requires triage labels Dec 17, 2023
@bmingles bmingles self-assigned this Dec 17, 2023
@vbabich vbabich added this to the December 2023 milestone Dec 19, 2023
@vbabich vbabich removed the triage Issue requires triage label Dec 19, 2023
@vbabich vbabich assigned mofojed and unassigned bmingles Dec 19, 2023
@bmingles bmingles assigned bmingles and unassigned mofojed Jan 8, 2024
@mofojed mofojed modified the milestones: December 2023, January 2024 Jan 9, 2024
bmingles added a commit to bmingles/web-client-ui that referenced this issue Jan 10, 2024
bmingles added a commit to bmingles/web-client-ui that referenced this issue Jan 10, 2024
bmingles added a commit to bmingles/web-client-ui that referenced this issue Jan 11, 2024
bmingles added a commit to bmingles/web-client-ui that referenced this issue Jan 11, 2024
bmingles added a commit that referenced this issue Jan 11, 2024
- Preload now always applies default preload values before applying
cached ones. This ensures that cached preload values don't prevent new
defaults from being applied if / when we add new variables to the
preload list
- Default preload variables can now be passed in to ThemeUtils +
ThemeProvider. This will allow DHE to specify additional variables if
needed

resolves #1695 and part of #1679
mofojed pushed a commit to deephaven/deephaven-core that referenced this issue Jan 17, 2024
# [0.59.0](deephaven/web-client-ui@v0.58.0...v0.59.0) (2024-01-17)


### Bug Fixes

* GoToRow timestamp fails when selected row is out of view ([#1717](deephaven/web-client-ui#1717)) ([9ddc973](deephaven/web-client-ui@9ddc973)), closes [#1561](deephaven/web-client-ui#1561)
* Interface for IrisGridTableModelTemplate.backgroundColorForCell ([#1699](deephaven/web-client-ui#1699)) ([73e1837](deephaven/web-client-ui@73e1837)), closes [#1697](deephaven/web-client-ui#1697)
* Moved logos so they show in production build ([#1713](deephaven/web-client-ui#1713)) ([a3bea73](deephaven/web-client-ui@a3bea73)), closes [#1712](deephaven/web-client-ui#1712)
* re-colorize command codeblocks when theme changes ([#1731](deephaven/web-client-ui#1731)) ([b1e42f5](deephaven/web-client-ui@b1e42f5))
* TimeInput not triggering onChange on incomplete values ([#1711](deephaven/web-client-ui#1711)) ([6894d96](deephaven/web-client-ui@6894d96)), closes [#1710](deephaven/web-client-ui#1710)


### Features

* Action button tooltips ([#1706](deephaven/web-client-ui#1706)) ([bff6bf9](deephaven/web-client-ui@bff6bf9)), closes [#1705](deephaven/web-client-ui#1705)
* Add support for useDeferredApi ([#1725](deephaven/web-client-ui#1725)) ([51ebe1b](deephaven/web-client-ui@51ebe1b))
* Improved preload variable handling ([#1723](deephaven/web-client-ui#1723)) ([ed41c42](deephaven/web-client-ui@ed41c42)), closes [#1695](deephaven/web-client-ui#1695) [#1679](deephaven/web-client-ui#1679)
* NavTabList component ([#1698](deephaven/web-client-ui#1698)) ([96641fb](deephaven/web-client-ui@96641fb))
* Reject promise immediately if var not found ([#1718](deephaven/web-client-ui#1718)) ([43d40bd](deephaven/web-client-ui@43d40bd)), closes [#1701](deephaven/web-client-ui#1701)
* theming tweaks ([#1727](deephaven/web-client-ui#1727)) ([f919a7e](deephaven/web-client-ui@f919a7e))


### BREAKING CHANGES

* - Subclasses of IrisGridTableModelTemplate or it's subclasses that use
backgroundColorForCell may need to update their signature to accept the
theme if they are calling the superclass


Co-authored-by: deephaven-internal <deephaven-internal@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants