-
-
Notifications
You must be signed in to change notification settings - Fork 3k
refactor(theme): remove duplicate color definitions (@fehmer) #7366
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
base: master
Are you sure you want to change the base?
Conversation
fb1b6f4 to
1bbea5d
Compare
|
@copilot only serika dark, darling and dots are converted on this pr. convert the rest |
2ec3e67 to
c4bd55f
Compare
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.
Pull request overview
This PR refactors theme handling by consolidating all color definitions from individual CSS files into a centralized TypeScript constants file. It removes now-empty CSS files, introduces a new Theme SolidJS component to dynamically apply CSS variables, and updates all related code to work with the new architecture.
Changes:
- Consolidated all theme color definitions into
frontend/src/ts/constants/themes.ts - Removed duplicate color CSS variable definitions from individual theme CSS files
- Created new Theme SolidJS component to dynamically apply CSS variables and load theme-specific CSS files
- Updated theme-related code throughout the application to use the new centralized theme colors
Reviewed changes
Copilot reviewed 215 out of 216 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/static/themes/*.css | Removed :root CSS variable blocks from all theme files (keeping only additional CSS rules) |
| frontend/src/ts/constants/themes.ts | Would need to verify this file contains all theme definitions |
| frontend/src/ts/signals/theme.ts | Updated to export Theme type and removed ThemeColors duplicate definition |
| frontend/src/ts/components/layout/Theme.tsx | New SolidJS component for managing theme CSS variables and loading theme-specific CSS |
| frontend/src/ts/elements/theme-colors.ts | Deleted file (functionality moved to signals) |
| frontend/src/ts/controllers/theme-controller.ts | Refactored to work with centralized theme definitions |
| frontend/src/ts/elements/settings/theme-picker.ts | Updated to use new theme color management approach |
| frontend/src/html/pages/settings.html | Updated color picker IDs to use data-key attributes |
| frontend/src/index.html | Removed hardcoded theme link |
| frontend/src/html/head.html | Added Theme component mount point |
| Multiple TS files | Updated imports and function calls to use new theme color getters |
Comments suppressed due to low confidence (1)
frontend/src/html/pages/settings.html:1606
- Inconsistent ID naming. This input has
id='--sub-color'but should beid='colorPickerSub'to match the naming pattern used by all other color picker inputs in the same settings panel.
id="--sub-color"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| end: ")", | ||
| }, | ||
| { | ||
| rule: /\b[A-Z, a-z, 0-9]{6}\b/, |
Copilot
AI
Jan 15, 2026
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.
Character ',' is repeated in the same character class.
| rule: /\b[A-Z, a-z, 0-9]{6}\b/, | |
| rule: /\b[A-Za-z0-9]{6}\b/, |
| end: ")", | ||
| }, | ||
| { | ||
| rule: /\b[A-Z, a-z, 0-9]{6}\b/, |
Copilot
AI
Jan 15, 2026
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.
Character ' ' is repeated in the same character class.
| rule: /\b[A-Z, a-z, 0-9]{6}\b/, | |
| rule: /\b[A-Za-z0-9]{6}\b/, |
Uh oh!
There was an error while loading. Please reload this page.