-
Notifications
You must be signed in to change notification settings - Fork 15.4k
feat: add a theme CRUD page to manage themes #34182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment
|
@@ -38,7 +38,6 @@ Set the feature flag in your `superset_config` | |||
```python | |||
DEFAULT_FEATURE_FLAGS: dict[str, bool] = { | |||
{{ ... }} | |||
THEME_ALLOW_THEME_EDITOR_BETA = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this feature flag now as we have an "apply" button in the theme editor in the new CRUD
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@mistercrunch Ephemeral environment spinning up at http://54.244.28.232:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.create_table( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're supposed to use the helper functions for creating tables and columns going forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, let me fix and see if I can add a linting rule (?) Though would love to spend some cycles improving migrations:
- compact migration before say 3.0 (wondering where to draw the line)
- making sure we use utils consistently
- add linting rules to prevent not using utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now it looks like we only use backend for storing the Theme config and name. It would be cool to add some kind of tagging system for light/dark to enable quick mode switching for users.
/** | ||
* 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(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is used anywhere. All of the crud related stuff in this file seems unused, we should probably implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right, though we might need it in the future (?) might be attached to user prefs in the future. Can keep for now.
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@mistercrunch Ephemeral environment spinning up at http://44.245.7.189:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@mistercrunch Ephemeral environment spinning up at http://54.203.44.180:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
superset-frontend/packages/superset-ui-core/src/components/ThemeSelect/index.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/packages/superset-ui-core/src/components/ThemeSelect/index.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we set some default tokens in packages/superset-ui-core/src/theme/Theme.tsx
But they are not visible in the ui for the theme and i am not sure if they are applied to database either. Should we move them to default config options instead?
The theme constructor is (AFAIK) deterministic (well kinda), meaning when you set your theme to |
@mistercrunch Processing your ephemeral environment request here. Action: up. More information on how to use or configure ephemeral environments |
@mistercrunch Ephemeral environment spinning up at http://54.190.54.11:8080. Credentials are 'admin'/'admin'. Please allow several minutes for bootstrapping and startup. |
Summary
This PR introduces a comprehensive theme management system for Superset with complete CRUD functionality and fixes critical theme scoping issues through a major architecture refactor.
Key Features
🎨 Theme CRUD Management
📊 Dashboard Theme Integration
🏗️ Architecture Refactor (Major New Work)
Problem: Dashboard themes were affecting entire app (including navbar) instead of just dashboard content.
Solution: Complete theme system overhaul:
themeObject
dependencies, components now use context-awareuseTheme()
+ utilitiesthemeUtils.ts
🔧 System Integration
THEME_ENABLE_DARK_THEME_SWITCH
defaults to TrueFixed Behaviors
Technical Implementation
Backend: Theme model, complete REST API, dashboard integration, system theme seeding
Frontend: CRUD interface, dashboard integration, proper theme scoping architecture
Component Library: Refactored to use context-aware theming without global dependencies
Testing: Comprehensive test coverage including new
themeUtils.test.ts
Migration & Breaking Changes
theme_id
columnuseTheme()
+ utilities instead of globalthemeObject
methodsConfiguration Examples
This implementation provides enterprise-grade theme management with proper isolation, security, and a solid foundation for advanced theming capabilities.
🤖 Co-generated with https://claude.ai/code