alexandrusoare commented on code in PR #35220: URL: https://github.com/apache/superset/pull/35220#discussion_r2391444755
########## superset/config.py: ########## @@ -723,46 +723,69 @@ class D3TimeFormat(TypedDict, total=False): # This is merely a default EXTRA_CATEGORICAL_COLOR_SCHEMES: list[dict[str, Any]] = [] -# --------------------------------------------------- -# Theme Configuration for Superset -# --------------------------------------------------- +# ----------------------------------------------------------------------------- +# Theme System Configuration +# ----------------------------------------------------------------------------- # Superset supports custom theming through Ant Design's theme structure. -# This allows users to customize colors, fonts, and other UI elements. -# -# Theme Generation: -# - Use the Ant Design theme editor: https://ant.design/theme-editor -# - Export or copy the generated theme JSON and assign to the variables below -# - For detailed instructions: https://superset.apache.org/docs/configuration/theming/ # -# To expose a JSON theme editor modal that can be triggered from the navbar -# set the `ENABLE_THEME_EDITOR` feature flag to True. +# Theme Hierarchy: +# 1. THEME_DEFAULT/THEME_DARK - Base themes defined in config (foundation) +# 2. System themes - Set by admins via UI (when ENABLE_UI_THEME_ADMINISTRATION=True) +# 3. Dashboard themes - Applied per dashboard using the theme bolt button # -# Theme Structure: -# Each theme should follow Ant Design's theme format. -# To create custom themes, use the Ant Design Theme Editor at https://ant.design/theme-editor -# and copy the generated JSON configuration. +# How it works: +# - Custom themes override base themes for any properties they define +# - Properties not defined in custom themes use the base theme values +# - Admins can set system-wide themes that apply to all users +# - Users can apply specific themes to individual dashboards # -# Example theme definition: -# THEME_DEFAULT = { -# "token": { -# "colorPrimary": "#2893B3", -# "colorSuccess": "#5ac189", -# "colorWarning": "#fcc700", -# "colorError": "#e04355", -# "fontFamily": "'Inter', Helvetica, Arial", -# ... # other tokens -# }, -# ... # other theme properties -# } - - -# Default theme configuration -# Leave empty to use Superset's default theme -THEME_DEFAULT: Theme = {"algorithm": "default"} +# Theme Creation: +# - Use the Ant Design theme editor: https://ant.design/theme-editor +# - Export the generated JSON and use it in your theme configuration +# ----------------------------------------------------------------------------- + +# Default theme configuration - foundation for all themes +# This acts as the base theme for all users +THEME_DEFAULT: Theme = { + "token": { + # Brand + "brandLogoAlt": "Apache Superset", + "brandLogoUrl": APP_ICON, + "brandLogoMargin": "18px", + "brandLogoHref": "/", + "brandLogoHeight": "24px", + # Spinner + "brandSpinnerUrl": None, + "brandSpinnerSvg": None, + # Default colors + "colorPrimary": "#2893B3", # NOTE: previous lighter primary color was #20a7c9 # noqa: E501 + "colorLink": "#2893B3", + "colorError": "#e04355", + "colorWarning": "#fcc700", + "colorSuccess": "#5ac189", + "colorInfo": "#66bcfe", + # Fonts + "fontFamily": "'Inter', Helvetica, Arial", + "fontFamilyCode": "'Fira Code', 'Courier New', monospace", Review Comment: Why does Inter needs ' '? ########## superset/daos/theme.py: ########## @@ -45,27 +45,14 @@ def find_system_default(cls) -> Optional[Theme]: if len(system_defaults) == 1: return system_defaults[0] - if len(system_defaults) > 1: - logger.warning( - "Multiple system default themes found (%s), " - "falling back to config theme", - len(system_defaults), - ) - - # Fallback to is_system=True theme with name 'THEME_DEFAULT' - return ( - db.session.query(Theme) - .filter(Theme.is_system.is_(True), Theme.theme_name == "THEME_DEFAULT") - .first() - ) + return None Review Comment: Why did we drop the logic if there are multiple themes found for either default or dark? ########## superset/config.py: ########## @@ -723,46 +723,69 @@ class D3TimeFormat(TypedDict, total=False): # This is merely a default EXTRA_CATEGORICAL_COLOR_SCHEMES: list[dict[str, Any]] = [] -# --------------------------------------------------- -# Theme Configuration for Superset -# --------------------------------------------------- +# ----------------------------------------------------------------------------- +# Theme System Configuration +# ----------------------------------------------------------------------------- # Superset supports custom theming through Ant Design's theme structure. -# This allows users to customize colors, fonts, and other UI elements. -# -# Theme Generation: -# - Use the Ant Design theme editor: https://ant.design/theme-editor -# - Export or copy the generated theme JSON and assign to the variables below -# - For detailed instructions: https://superset.apache.org/docs/configuration/theming/ # -# To expose a JSON theme editor modal that can be triggered from the navbar -# set the `ENABLE_THEME_EDITOR` feature flag to True. +# Theme Hierarchy: +# 1. THEME_DEFAULT/THEME_DARK - Base themes defined in config (foundation) +# 2. System themes - Set by admins via UI (when ENABLE_UI_THEME_ADMINISTRATION=True) +# 3. Dashboard themes - Applied per dashboard using the theme bolt button # -# Theme Structure: -# Each theme should follow Ant Design's theme format. -# To create custom themes, use the Ant Design Theme Editor at https://ant.design/theme-editor -# and copy the generated JSON configuration. +# How it works: +# - Custom themes override base themes for any properties they define +# - Properties not defined in custom themes use the base theme values +# - Admins can set system-wide themes that apply to all users +# - Users can apply specific themes to individual dashboards # -# Example theme definition: -# THEME_DEFAULT = { -# "token": { -# "colorPrimary": "#2893B3", -# "colorSuccess": "#5ac189", -# "colorWarning": "#fcc700", -# "colorError": "#e04355", -# "fontFamily": "'Inter', Helvetica, Arial", -# ... # other tokens -# }, -# ... # other theme properties -# } - - -# Default theme configuration -# Leave empty to use Superset's default theme -THEME_DEFAULT: Theme = {"algorithm": "default"} +# Theme Creation: +# - Use the Ant Design theme editor: https://ant.design/theme-editor +# - Export the generated JSON and use it in your theme configuration +# ----------------------------------------------------------------------------- + +# Default theme configuration - foundation for all themes +# This acts as the base theme for all users +THEME_DEFAULT: Theme = { + "token": { + # Brand + "brandLogoAlt": "Apache Superset", + "brandLogoUrl": APP_ICON, + "brandLogoMargin": "18px", + "brandLogoHref": "/", + "brandLogoHeight": "24px", + # Spinner + "brandSpinnerUrl": None, + "brandSpinnerSvg": None, + # Default colors + "colorPrimary": "#2893B3", # NOTE: previous lighter primary color was #20a7c9 # noqa: E501 + "colorLink": "#2893B3", + "colorError": "#e04355", + "colorWarning": "#fcc700", + "colorSuccess": "#5ac189", + "colorInfo": "#66bcfe", + # Fonts + "fontFamily": "'Inter', Helvetica, Arial", + "fontFamilyCode": "'Fira Code', 'Courier New', monospace", + # Extra tokens + "transitionTiming": 0.3, + "brandIconMaxWidth": 37, + "fontSizeXS": "8", + "fontSizeXXL": "28", + "fontWeightNormal": "400", + "fontWeightLight": "300", + "fontWeightStrong": 500, Review Comment: ```suggestion "fontWeightStrong": "500", ``` ########## superset/views/base.py: ########## @@ -311,66 +313,86 @@ def menu_data(user: User) -> dict[str, Any]: } +def _merge_theme_dicts(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any]: + """ + Recursively merge overlay theme dict into base theme dict. + Arrays and non-dict values are replaced, not merged. + """ + result = base.copy() + for key, value in overlay.items(): + if isinstance(result.get(key), dict) and isinstance(value, dict): + result[key] = _merge_theme_dicts(result[key], value) + else: + result[key] = value + return result + + +def _load_theme_from_model( + theme_model: ThemeModel | None, + fallback_theme: Theme | None, + theme_type: str, +) -> Theme | None: + """Load and parse theme from database model, merging with config theme as base.""" + if theme_model: + try: + db_theme = json.loads(theme_model.json_data) + if fallback_theme: + merged = _merge_theme_dicts(dict(fallback_theme), db_theme) + return cast(Theme, merged) + return db_theme + except json.JSONDecodeError: + logger.error( + "Invalid JSON in system %s theme %s", theme_type, theme_model.id + ) + return fallback_theme + return fallback_theme + + +def _process_theme(theme: Theme | None, theme_type: str) -> Theme: + """Process and validate a theme, returning an empty dict if invalid.""" + if theme is None or theme == {}: + # When config theme is None or empty, don't provide a custom theme + # The frontend will use base theme only + return {} + elif not is_valid_theme(cast(dict[str, Any], theme)): + logger.warning( + "Invalid %s theme configuration: %s, clearing it", + theme_type, + theme, + ) + return {} + return theme or {} + + def get_theme_bootstrap_data() -> dict[str, Any]: """ Returns the theme data to be sent to the client. """ # Check if UI theme administration is enabled enable_ui_admin = app.config.get("ENABLE_UI_THEME_ADMINISTRATION", False) + # Get config themes to use as fallback + config_theme_default = get_config_value("THEME_DEFAULT") + config_theme_dark = get_config_value("THEME_DARK") + if enable_ui_admin: # Try to load themes from database default_theme_model = ThemeDAO.find_system_default() dark_theme_model = ThemeDAO.find_system_dark() # Parse theme JSON from database models - default_theme = {} - if default_theme_model: - try: - default_theme = json.loads(default_theme_model.json_data) - except json.JSONDecodeError: - logger.error( - "Invalid JSON in system default theme %s", - default_theme_model.id, - ) - # Fallback to config - default_theme = get_config_value("THEME_DEFAULT") - else: - # No system default theme in database, use config - default_theme = get_config_value("THEME_DEFAULT") - - dark_theme = {} - if dark_theme_model: - try: - dark_theme = json.loads(dark_theme_model.json_data) - except json.JSONDecodeError: - logger.error( - "Invalid JSON in system dark theme %s", dark_theme_model.id - ) - # Fallback to config - dark_theme = get_config_value("THEME_DARK") - else: - # No system dark theme in database, use config - dark_theme = get_config_value("THEME_DARK") - else: - # UI theme administration disabled, use config-based themes - default_theme = get_config_value("THEME_DEFAULT") - dark_theme = get_config_value("THEME_DARK") - - # Validate theme configurations - if not is_valid_theme(default_theme): - logger.warning( - "Invalid default theme configuration: %s, using empty theme", - default_theme, + default_theme = _load_theme_from_model( + default_theme_model, config_theme_default, "default" ) - default_theme = {} + dark_theme = _load_theme_from_model(dark_theme_model, config_theme_dark, "dark") + else: + # UI theme administration disabled - use config-based themes + default_theme = config_theme_default + dark_theme = config_theme_dark - if not is_valid_theme(dark_theme): - logger.warning( - "Invalid dark theme configuration: %s, using empty theme", - dark_theme, - ) - dark_theme = {} + # Process and validate themes + default_theme = _process_theme(default_theme, "default") + dark_theme = _process_theme(dark_theme, "dark") Review Comment: Maybe we can use enums for `dark`, `light`, `default` etc ########## superset/config.py: ########## @@ -723,46 +723,69 @@ class D3TimeFormat(TypedDict, total=False): # This is merely a default EXTRA_CATEGORICAL_COLOR_SCHEMES: list[dict[str, Any]] = [] -# --------------------------------------------------- -# Theme Configuration for Superset -# --------------------------------------------------- +# ----------------------------------------------------------------------------- +# Theme System Configuration +# ----------------------------------------------------------------------------- # Superset supports custom theming through Ant Design's theme structure. -# This allows users to customize colors, fonts, and other UI elements. -# -# Theme Generation: -# - Use the Ant Design theme editor: https://ant.design/theme-editor -# - Export or copy the generated theme JSON and assign to the variables below -# - For detailed instructions: https://superset.apache.org/docs/configuration/theming/ # -# To expose a JSON theme editor modal that can be triggered from the navbar -# set the `ENABLE_THEME_EDITOR` feature flag to True. +# Theme Hierarchy: +# 1. THEME_DEFAULT/THEME_DARK - Base themes defined in config (foundation) +# 2. System themes - Set by admins via UI (when ENABLE_UI_THEME_ADMINISTRATION=True) +# 3. Dashboard themes - Applied per dashboard using the theme bolt button # -# Theme Structure: -# Each theme should follow Ant Design's theme format. -# To create custom themes, use the Ant Design Theme Editor at https://ant.design/theme-editor -# and copy the generated JSON configuration. +# How it works: +# - Custom themes override base themes for any properties they define +# - Properties not defined in custom themes use the base theme values +# - Admins can set system-wide themes that apply to all users +# - Users can apply specific themes to individual dashboards # -# Example theme definition: -# THEME_DEFAULT = { -# "token": { -# "colorPrimary": "#2893B3", -# "colorSuccess": "#5ac189", -# "colorWarning": "#fcc700", -# "colorError": "#e04355", -# "fontFamily": "'Inter', Helvetica, Arial", -# ... # other tokens -# }, -# ... # other theme properties -# } - - -# Default theme configuration -# Leave empty to use Superset's default theme -THEME_DEFAULT: Theme = {"algorithm": "default"} +# Theme Creation: +# - Use the Ant Design theme editor: https://ant.design/theme-editor +# - Export the generated JSON and use it in your theme configuration +# ----------------------------------------------------------------------------- + +# Default theme configuration - foundation for all themes +# This acts as the base theme for all users +THEME_DEFAULT: Theme = { + "token": { + # Brand + "brandLogoAlt": "Apache Superset", + "brandLogoUrl": APP_ICON, + "brandLogoMargin": "18px", + "brandLogoHref": "/", + "brandLogoHeight": "24px", + # Spinner + "brandSpinnerUrl": None, + "brandSpinnerSvg": None, + # Default colors + "colorPrimary": "#2893B3", # NOTE: previous lighter primary color was #20a7c9 # noqa: E501 + "colorLink": "#2893B3", + "colorError": "#e04355", + "colorWarning": "#fcc700", + "colorSuccess": "#5ac189", + "colorInfo": "#66bcfe", + # Fonts + "fontFamily": "'Inter', Helvetica, Arial", + "fontFamilyCode": "'Fira Code', 'Courier New', monospace", + # Extra tokens + "transitionTiming": 0.3, + "brandIconMaxWidth": 37, + "fontSizeXS": "8", + "fontSizeXXL": "28", Review Comment: Since ant design has changed the fontSizeXS from 12 to 14, maybe we should use 14 as well, just a nit ########## superset-frontend/src/pages/ThemeList/index.tsx: ########## @@ -235,60 +253,83 @@ function ThemesList({ }; // Generic confirmation modal utility to reduce code duplication - const showThemeConfirmation = (config: { - title: string; - content: string; - onConfirm: () => Promise<any>; - successMessage: string; - errorMessage: string; - }) => { - Modal.confirm({ - title: config.title, - content: config.content, - onOk: () => { - config - .onConfirm() - .then(() => { - refreshData(); - addSuccessToast(config.successMessage); - }) - .catch(err => { - addDangerToast(t(config.errorMessage, err.message)); - }); - }, - }); - }; + const showThemeConfirmation = useCallback( + (config: { + title: string; + content: string; + onConfirm: () => Promise<any>; + successMessage: string; + errorMessage: string; + }) => { + setConfirmModalConfig({ Review Comment: ditto, having a type for the config would be a nice nit ########## superset-frontend/src/pages/ThemeList/index.tsx: ########## @@ -112,6 +117,16 @@ function ThemesList({ const [importingTheme, showImportModal] = useState<boolean>(false); const [appliedThemeId, setAppliedThemeId] = useState<number | null>(null); + // State for confirmation modal + const [confirmModalConfig, setConfirmModalConfig] = useState<{ + visible: boolean; + title: string; + message: string; + onConfirm: () => Promise<any>; + successMessage: string; + errorMessage: string; + } | null>(null); + Review Comment: Maybe it would be better to have a separate type for this state, something like `confirmModalConfigType` -- 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]
