-
Notifications
You must be signed in to change notification settings - Fork 67
fix (color schemes): remove color scheme styles in editor if there are no color schemes #3622
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
WalkthroughAdds a computed guard (addContainerSchemeDefaultColors) to set default colors for container schemes when background/container schemes are missing or conflicting, preventing unintended inheritance. Updates a conditional branch to use this flag while leaving render flow and CSS generation intact. No public APIs or exports changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Editor UI
participant Loader as editor-loader
participant Schemes as ColorSchemes
UI->>Loader: loadSchemes()
Loader->>Schemes: get backgroundScheme, containerScheme
Schemes-->>Loader: schemes data
alt addContainerSchemeDefaultColors = true
note right of Loader: New guard computes default colors<br/>for container to avoid fallback to background
Loader->>Loader: apply container default colors
else
Loader->>Loader: keep existing container colors
end
Loader-->>UI: render with computed CSS
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/plugins/global-settings/color-schemes/editor-loader.js (1)
72-79: Good fix for the inheritance issue; consider clarifying the length check.The computed guard correctly prevents containers from inheriting background colors that could make text unreadable. The main logic (checking if background scheme has value while container is empty) is solid.
However, the
colorSchemesArray.length !== 2condition is somewhat fragile. It assumes a specific array structure and doesn't explicitly verify what those 2 schemes are. This could break if the array structure changes or if there are different combinations of schemes (e.g., default + container instead of default + background).Consider replacing the length check with a more explicit condition that describes what you're actually checking for. For example:
- // This fixes the issue wherein if there is a background scheme and no container/base scheme, the container inherits the background scheme which may cause the text to be unreadable - const addContainerSchemeDefaultColors = containerModeColorScheme in colorSchemes && ! schemeHasValue( colorSchemes[ containerModeColorScheme ] ) && - ( - // Add default container scheme if background scheme has value - ( backgroundModeColorScheme in colorSchemes && schemeHasValue( colorSchemes[ backgroundModeColorScheme ] ) ) || - // Add default container scheme if there are color schemes other than the default scheme and background scheme - ( colorSchemesArray.length !== 2 ) - ) + // This fixes the issue wherein if there is a background scheme and no container/base scheme, + // the container inherits the background scheme which may cause the text to be unreadable + const hasBackgroundSchemeWithValue = backgroundModeColorScheme in colorSchemes && + schemeHasValue( colorSchemes[ backgroundModeColorScheme ] ) + const hasCustomSchemes = colorSchemesArray.some( scheme => + scheme.slug !== baseColorScheme && + scheme.slug !== backgroundModeColorScheme && + schemeHasValue( scheme ) + ) + const addContainerSchemeDefaultColors = + containerModeColorScheme in colorSchemes && + ! schemeHasValue( colorSchemes[ containerModeColorScheme ] ) && + ( hasBackgroundSchemeWithValue || hasCustomSchemes )This makes the intent clearer and is more resilient to changes in array structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/plugins/global-settings/color-schemes/editor-loader.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/plugins/global-settings/color-schemes/editor-loader.js (1)
src/plugins/global-settings/color-schemes/utils.js (2)
schemeHasValue(29-35)schemeHasValue(29-35)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: build
🔇 Additional comments (1)
src/plugins/global-settings/color-schemes/editor-loader.js (1)
91-91: LGTM! Cleaner conditional structure.Using the computed
addContainerSchemeDefaultColorsflag instead of inline complex logic improves readability and maintainability.
Summary by CodeRabbit