rusackas commented on code in PR #42602:
URL: https://github.com/apache/superset/pull/42602#discussion_r3707300478
##########
docs/src/components/StorybookWrapper.jsx:
##########
@@ -78,15 +80,39 @@ function getProviders() {
return container || document.body;
};
+ // `themeObject` is a module-level singleton (superset-core/src/theme
+ // index.tsx: `Theme.fromConfig()`), created once with no dark/light
+ // config, so SupersetThemeProvider always rendered whatever that default
+ // algorithm was -- it had no way to know about Docusaurus's theme toggle.
+ // Docusaurus tracks the toggle in React context (useColorMode), so
+ // mirror it onto the singleton via the toggleDarkMode() method Theme
+ // already exposes for exactly this purpose.
+ //
+ // Use useLayoutEffect (not useEffect) so the sync runs before the
+ // browser paints. This component only ever mounts client-side (it's
+ // built inside a BrowserOnly callback), so there's no SSR mismatch
+ // concern -- and running synchronously before paint avoids a brief
+ // flash of the singleton's previous palette when a page loads directly
+ // in dark mode or the toggle fires during route navigation.
+ function ThemeSync({ children }) {
+ const { colorMode } = useColorMode();
+ React.useLayoutEffect(() => {
Review Comment:
Good catch. Made `toggleDarkMode` bail out early when the requested mode is
already active, so only the first of the N bridge calls per toggle actually
recomputes and notifies providers, the rest are now cheap no-ops. Pushed in
d0d11baf. Left the bridges themselves as one-per-demo since consolidating those
is a bigger change.
##########
docs/src/components/StorybookWrapper.jsx:
##########
@@ -78,15 +80,39 @@ function getProviders() {
return container || document.body;
};
+ // `themeObject` is a module-level singleton (superset-core/src/theme
+ // index.tsx: `Theme.fromConfig()`), created once with no dark/light
+ // config, so SupersetThemeProvider always rendered whatever that default
+ // algorithm was -- it had no way to know about Docusaurus's theme toggle.
+ // Docusaurus tracks the toggle in React context (useColorMode), so
+ // mirror it onto the singleton via the toggleDarkMode() method Theme
+ // already exposes for exactly this purpose.
+ //
+ // Use useLayoutEffect (not useEffect) so the sync runs before the
+ // browser paints. This component only ever mounts client-side (it's
+ // built inside a BrowserOnly callback), so there's no SSR mismatch
+ // concern -- and running synchronously before paint avoids a brief
+ // flash of the singleton's previous palette when a page loads directly
+ // in dark mode or the toggle fires during route navigation.
+ function ThemeSync({ children }) {
+ const { colorMode } = useColorMode();
+ React.useLayoutEffect(() => {
Review Comment:
Good catch. Made `toggleDarkMode` bail out early when the requested mode is
already active, so only the first of the N bridge calls per toggle actually
recomputes and notifies providers, the rest are now cheap no-ops. Left the
bridges themselves as one-per-demo since consolidating those is a bigger change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
