fix: default theme colors cannot be overridden #4636
Conversation
|
WalkthroughThe changes involve modifications to the Changes
Assessment against linked issues
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/@core/preferences/src/update-css-variables.ts (1)
Line range hint
38-63: Consider refactoring for clarity and consistencyWhile the changes address the issue of overriding default colors, consider the following improvements:
- The
builtinTypeColorPrimaryis calculated but not directly used. Consider removing or refactoring this logic if it's no longer needed.- Ensure that
updateMainColorVariablesuses the correct color values, considering thatpreferences.theme.colorPrimaryis no longer updated in this function.Here's a suggested refactor to improve clarity:
let builtinTypeColorPrimary: string | undefined = ''; if (currentBuiltType) { const isDark = isDarkTheme(preferences.theme.mode); // 设置不同主题的主要颜色 const color = isDark ? currentBuiltType.darkPrimaryColor || currentBuiltType.primaryColor : currentBuiltType.primaryColor; builtinTypeColorPrimary = color || currentBuiltType.color; } // 如果内置主题颜色和自定义颜色都不存在，则不更新主题颜色 if ( - builtinTypeColorPrimary || Reflect.has(theme, 'colorPrimary') || Reflect.has(theme, 'colorDestructive') || Reflect.has(theme, 'colorSuccess') || Reflect.has(theme, 'colorWarning') ) { - // preferences.theme.colorPrimary = builtinTypeColorPrimary || colorPrimary; updateMainColorVariables(preferences); }This change removes the unused
builtinTypeColorPrimaryfrom the condition and simplifies the logic. Please ensure thatupdateMainColorVariablescorrectly handles the case wherecolorPrimarymight not be set inpreferences.theme.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/@core/preferences/src/update-css-variables.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
packages/@core/preferences/src/update-css-variables.ts (3)
Line range hint
1-114: Summary: Changes effectively address the theme color override issueThe modifications in this file successfully address the reported bug where default theme colors couldn't be overridden. The key changes involve:
- Removing
colorPrimaryfrom the destructured theme object.- Commenting out the assignment to
preferences.theme.colorPrimary.These changes allow user-defined color preferences to take precedence over built-in theme colors. However, there are a few suggestions for improvement:
- Consider refactoring the
builtinTypeColorPrimarylogic if it's no longer needed.- Ensure that
updateMainColorVariablescorrectly handles cases wherecolorPrimarymight not be set inpreferences.theme.- Review the entire theme system to ensure consistency with these changes.
To ensure these changes don't introduce any regressions, please run the following verification:
#!/bin/bash # Description: Verify the impact of changes on the theme system # Test: Check for any remaining direct assignments to theme colors echo "Checking for direct assignments to theme colors:" rg --type typescript 'theme\.(colorPrimary|colorDestructive|colorSuccess|colorWarning)\s*=' # Test: Verify the usage of updateMainColorVariables echo "Checking usage of updateMainColorVariables:" rg --type typescript 'updateMainColorVariables' # Test: Look for any potential theme-related TODOs or FIXMEs echo "Checking for theme-related TODOs or FIXMEs:" rg --type typescript '(TODO|FIXME).*theme'After addressing the suggestions and running the verification, this change should be ready for merging.
61-61: Approved: Commenting out assignment to preferences.theme.colorPrimaryThis change prevents the function from overwriting the user-defined
colorPrimary, allowing for theme color customization as intended.To ensure this change doesn't introduce any unintended side effects, please run the following verification:
#!/bin/bash # Description: Check for any code that might rely on preferences.theme.colorPrimary being set in this function # Test: Search for references to preferences.theme.colorPrimary echo "Searching for references to preferences.theme.colorPrimary:" rg --type typescript 'preferences\.theme\.colorPrimary' # Test: Search for other assignments to preferences.theme.colorPrimary echo "Searching for other assignments to preferences.theme.colorPrimary:" ast-grep --lang typescript --pattern 'preferences.theme.colorPrimary = $_;'
21-21: Approved: Removal ofcolorPrimaryfrom destructured theme objectThis change addresses the issue of default theme colors not being overridable by preventing the built-in theme color from overwriting the user-defined
colorPrimary.To ensure this change doesn't affect other parts of the codebase, please run the following verification:
✅ Verification successful
To address the issues with the previous shell scripts, please run the following updated verification scripts:
To resolve the issues encountered with the previous shell scripts, please run the following updated verification scripts:
Verified: No active references to
theme.colorPrimaryfound in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to theme.colorPrimary that might be affected by this change # Test: Search for references to theme.colorPrimary echo "Searching for references to theme.colorPrimary:" rg --type typescript 'theme\.colorPrimary' # Test: Search for other uses of colorPrimary in destructuring assignments echo "Searching for other uses of colorPrimary in destructuring assignments:" ast-grep --lang typescript --pattern 'const { $$$, colorPrimary, $$$ } = $_;'Length of output: 592
Script:
#!/bin/bash # Description: Check for any remaining references to theme.colorPrimary and related theme properties in TypeScript files. # Test: Search for references to theme.colorPrimary in .ts and .tsx files echo "Searching for references to theme.colorPrimary in TypeScript files:" rg --glob '*.ts' --glob '*.tsx' 'theme\.colorPrimary' # Test: Search for other uses of colorPrimary in destructuring assignments within .ts and .tsx files echo "Searching for destructuring assignments that include colorPrimary in TypeScript files:" ast-grep --lang typescript --pattern 'const { $$$, colorPrimary, $$$ } = $_;' -- *.ts *.tsxLength of output: 700
Script:
#!/bin/bash # Description: Check for any remaining references to theme.colorPrimary and related theme properties in TypeScript (.ts) files. # Test: Search for references to theme.colorPrimary in .ts files echo "Searching for references to theme.colorPrimary in TypeScript (.ts) files:" rg --glob '*.ts' 'theme\.colorPrimary' # Test: Search for other uses of colorPrimary in destructuring assignments within .ts files echo "Searching for destructuring assignments that include colorPrimary in TypeScript (.ts) files:" ast-grep --lang typescript --pattern 'const { $$$, colorPrimary, $$$ } = $_;' -- *.tsLength of output: 648
Description
fixed #4613
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Bug Fixes