feat: Theming - Moved ThemeProvider updates into effect #1682
Conversation
bmingles
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1682 +/- ##
==========================================
+ Coverage 46.71% 46.73% +0.02%
==========================================
Files 606 606
Lines 36769 36757 -12
Branches 9231 9227 -4
==========================================
+ Hits 17176 17179 +3
+ Misses 19541 19526 -15
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. |
| } | ||
| }, [activeThemes]); | ||
| const chartTheme = useMemo( | ||
| () => (activeThemes == null ? null : defaultChartTheme()), |
There was a problem hiding this comment.
This doesn't seem right...
Looking at ChartModelFactory.makeModel, it's always expecting a non-null theme. Why are we allowing the theme to be null here? Shouldn't it always call defaultChartTheme() and possibly even throw if it's null?
Same comment under IrisGridThemeProvider.
There was a problem hiding this comment.
The activeThemes needs to remain null until plugins load to differentiate pending load vs an empty array that would designate plugins loaded, but no custom themes. We could use the default chart theme / grid theme, but seemed like unnecessary parsing when we know loading isn't complete yet. The IrisGridThemeContext and ChartThemeContext support null for the theme, but the consumers can opt to throw or provided a default if theme is null. useChartTheme throws if null, while IrisGrid handles the default if no theme is present.
There was a problem hiding this comment.
Ah right, this is just in the bootstrapping process that it's null.
Wondering if we should just change PluginsBootstrap to not render children until plugins are actually loaded; we need it for auth and for theming, which are the next things that get loaded ... then we wouldn't have to worry about having null plugins here. What do you think?
The initialization order in AppBootstrap.tsx could be something like:
<ClientBootstrap ...>
<PluginsBootstrap ...>
<ThemeBootstrap ...>
There was a problem hiding this comment.
I think that would work in DHC, but IIR, I think it bites us in DHE due to Login component being further down the tree. I guess they don't have to be implemented the same, but removing the possibility of things being null may cause us trouble trying to use the providers in DHE since they will have to support null for some cases.
There was a problem hiding this comment.
We're not even using PluginsBootstrap or PluginsContext in DHE... and yeah on DHE we can't load worker plugins until user is actually authenticated. We will need to wire up PluginsContext there at some point... regardless, why would we ever want to return null here? I think that's breaking the contract of what this bootstrap component should do, and may make it confusing to debug later.
I'd gate PluginsBootstrap, and these as well so there's never nulls. On Enterprise, it'll need to have it's own plugin bootstrapping step.
bmingles
force-pushed
the
1681-move-theme-provider-updates
branch
from
3ab65c8 to
b689d43
Compare
Moved ThemeProvider updates into effect
resolves #1681