mistercrunch commented on code in PR #35220:
URL: https://github.com/apache/superset/pull/35220#discussion_r2373290858
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -698,18 +743,38 @@ export class ThemeController {
* Applies the current theme configuration to the global theme.
* This method sets the theme on the globalTheme and applies it to the Theme.
* It also handles any errors that may occur during the application of the
theme.
- * @param theme - The theme configuration to apply
+ * @param theme - The theme configuration to apply (may already include base
theme tokens)
*/
private applyTheme(theme: AnyThemeConfig): void {
try {
const normalizedConfig = normalizeThemeConfig(theme);
+
+ // Simply apply the theme - it should already be properly merged if
needed
+ // The merging with base theme happens in getThemeForMode() and other
methods
+ // that prepare themes before passing them to applyTheme()
this.globalTheme.setConfig(normalizedConfig);
} catch (error) {
console.error('Failed to apply theme:', error);
this.fallbackToDefaultMode();
}
}
+ /**
+ * Determines if a theme configuration is for dark mode.
+ * @param theme - The theme configuration to check
+ * @returns True if the theme is dark mode, false otherwise
+ */
+ private isThemeDark(theme: AnyThemeConfig): boolean {
Review Comment:
oh right. Though in theory could potentially have a dark theme without the
dark algorithm, though highly unlikely/impractical. I think the main Theme
class has or used to have an isDark method too.
`theme.fromConfig(themeConfig).isDark` or similar might work too but require a
bit more computation.
there's a subtle difference between a theme config and a computed theme,
probably the typing system should be aware of the difference (maybe it is
already, I'd have to check) ... The Theme class knows how to generate one from
the other. There might be two `isThemeConfigDark` and `isThemeDark` .... If we
need both maybe they both live in utils.
I'm not sure what makes sense to do in the context of this PR, but might be
good to sort this out.
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
