msyavuz commented on code in PR #34182:
URL: https://github.com/apache/superset/pull/34182#discussion_r2215977089
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -43,6 +43,8 @@ const DEFAULT_THEME_SETTINGS = {
const STORAGE_KEYS = {
THEME_MODE: 'superset-theme-mode',
Review Comment:
Is this also irrelevant now? It stays as default in local storage
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -236,6 +246,76 @@ export class ThemeController {
this.updateTheme(defaultTheme);
}
+ /**
+ * Sets a CRUD theme by ID. This will fetch the theme from the API and apply
it.
+ * @param themeId - The ID of the CRUD theme to apply
+ */
+ public async setCrudTheme(themeId: string | null): Promise<void> {
+ this.crudThemeId = themeId;
+
+ if (themeId) {
+ this.storage.setItem(STORAGE_KEYS.CRUD_THEME_ID, themeId);
+ try {
+ const themeConfig = await this.fetchCrudTheme(themeId);
+ if (themeConfig) {
+ this.updateTheme(themeConfig);
+ }
+ } catch (error) {
+ console.error('Failed to load CRUD theme:', error);
+ this.fallbackToDefaultMode();
+ }
+ } else {
+ this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID);
+ this.resetTheme();
+ }
+ }
+
+ /**
+ * Sets a temporary theme override for development purposes.
+ * This does not persist the theme but allows live preview.
+ * @param theme - The theme configuration to apply temporarily
+ */
+ public setTemporaryTheme(theme: AnyThemeConfig): void {
+ this.validateThemeUpdatePermission();
+
+ this.devThemeOverride = theme;
+ this.storage.setItem(
+ STORAGE_KEYS.DEV_THEME_OVERRIDE,
+ JSON.stringify(theme),
+ );
+
+ const normalizedTheme = this.normalizeTheme(theme);
+ this.updateTheme(normalizedTheme);
+ }
Review Comment:
This persists the theme? This is what's called when clicked on apply and it
persists the theme.
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -236,6 +246,76 @@ export class ThemeController {
this.updateTheme(defaultTheme);
}
+ /**
+ * Sets a CRUD theme by ID. This will fetch the theme from the API and apply
it.
+ * @param themeId - The ID of the CRUD theme to apply
+ */
+ public async setCrudTheme(themeId: string | null): Promise<void> {
+ this.crudThemeId = themeId;
+
+ if (themeId) {
+ this.storage.setItem(STORAGE_KEYS.CRUD_THEME_ID, themeId);
+ try {
+ const themeConfig = await this.fetchCrudTheme(themeId);
+ if (themeConfig) {
+ this.updateTheme(themeConfig);
+ }
+ } catch (error) {
+ console.error('Failed to load CRUD theme:', error);
+ this.fallbackToDefaultMode();
+ }
+ } else {
+ this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID);
+ this.resetTheme();
+ }
+ }
Review Comment:
I don't think this is used anywhere. All of the crud related stuff in this
file seems unused, we should probably implement it.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -215,6 +222,34 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
return () => {};
}, [css]);
+ // Apply dashboard theme when dashboard loads
+ useEffect(() => {
+ const applyDashboardTheme = async () => {
+ if (dashboard?.theme?.id) {
+ try {
+ const response = await SupersetClient.get({
+ endpoint: `/api/v1/theme/${dashboard.theme.id}`,
+ });
+ const themeConfig = JSON.parse(response.json.result.json_data);
+ themeContext.setTemporaryTheme(themeConfig);
Review Comment:
This doesn't differentiate between dashboard themes and appwide themes.
Setting a theme from dashboard changes the whole app, was this intended?
--
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]
