-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Theming - Bootstrap #1603
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1603 +/- ##
==========================================
+ Coverage 46.62% 46.71% +0.09%
==========================================
Files 602 606 +4
Lines 36726 36769 +43
Branches 9208 9231 +23
==========================================
+ Hits 17122 17176 +54
+ Misses 19552 19541 -11
Partials 52 52
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
02ced7a
to
ba444f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things I noticed while testing:
- The animation on login screen (
RandomAreaPlotAnimation
) doesn't display anymore, it's just a black screen. - The first time I ran this, the loading circle was black... I think because I switched from
main
to this branch? Can't seem to reproduce it with an incognito window or by re-opening the same thing, so probably not a big deal but thought I'd mention it.
packages/components/src/theme/theme-dark/theme-dark-components.css
Outdated
Show resolved
Hide resolved
For the most part good, just the 404 link to bootstrap.scss and some minor cleanup suggestions to take a look at. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should cleanup the normalizeCssColor
, other comments are suggestions.
| '--dh-color-login-form-bg' | ||
| '--dh-color-login-status-message' | ||
| '--dh-color-random-area-plot-animation-fg-fill' | ||
| '--dh-color-random-area-plot-animation-fg-stroke' | ||
| '--dh-color-random-area-plot-animation-bg' | ||
| '--dh-color-random-area-plot-animation-grid' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells to me. ThemeModel
to me shouldn't have any specific things for login
or random-area-plot-animation
in my mind; rather login/random area plot should be pulling from generic values; e.g. instead of login-form-bg
, should just be form-bg
and that's what we use for all forms; login-status-message
should just be status-message
; random-area-plot-animation-fg-fill
should just be using accent-bg
, etc. Just seems like we're not doing the right abstraction here.
Then getRandomAreaPlotAnimationThemeColors()
would be a utility method within the RandomPlotAnimation rather than from the ThemeModel itself, which should just provide the generic colors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like @dsmmcken is saying the form colors should be specific to the form, so the issue here may be that the ThemeModel should not be responsible to specify these. I think these may not actually be required in the preload in DHC at all but will be needed for DHE, so I probably need to augment the preload to allow the app to specify additional vars. Then ThemeModel won't have to own these.
The other suggestions make sense. I'll change those as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This turned out to be more involved than I want to include in this PR. I created #1679 to address it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One suggestion for cleanup
const [themeColors, setThemeColors] = useState<ThemeColors | null>(null); | ||
|
||
useEffect(() => { | ||
setThemeColors(getRandomAreaPlotAnimationThemeColors()); | ||
}, [activeThemes]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should just memoize themeColors
, then you don't need checks for null
on themeColors
, e.g.:
const themeColors = useMemo(getRandomAreaPlotAnimationThemeColors, [
activeThemes,
]);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the places that depend on resolving css vars need to go in an effect so that the latest data is available to the DOM right after a theme selection change.
I'm not 100% sure, but I believe this happens because ThemeProvider renders style tags dynamically which would be in the same render as when useMemo gets calculated vs useEffect would fire after the DOM has actually updated the colors that are needed for the resolve.
For this particular example, it's easiest to see in the styleguide. When I tried useMemo and change the theme, the change trails behind till the next render
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a clarifying comment to the effect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay - as we discussed in our meeting it would be nice if we pulled this logic up to the ThemeProvider
so we don't need to do this pattern in so many places (here, ChartThemeProvider
, IrisGridThemeProvider
, etc).
So in ThemeProvider
, the value
it provides to the ThemeContext.Provider
is only set after a useLayoutEffect
occurs in the ThemeProvider
. Then any child can just do useActiveThemes
(or maybe a useThemeColors
type of hook?) and just get the active colours without needed to add their own useLayoutEffect
Can be a follow up ticket.
BREAKING CHANGE: Bootstrap color variables are now predominantly hsl based. SCSS will need to be updated accordingly. Theme providers are needed to load themes.