Fix text color handling in custom themes #519
Conversation
WalkthroughThe pull request refines the theming system by restructuring the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant D as DialogBox
participant TM as Theme Manager
participant T as Themer Module
participant C as Canvas
U->>D: Trigger input event
D->>D: Execute changeCustomTheme(e)
D->>TM: Update customTheme in list
TM->>T: Call updateThemeForStyle(themeName)
T->>document: Update HTML CSS properties (incl. --text)
T->>C: Set timeout to call renderCanvas()
C-->>T: Canvas refresh complete
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 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 (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
v0/src/simulator/src/themer/themer.js (1)
104-109: Consider using optional chaining for window.globalScopeThe forced canvas refresh ensures that text elements update immediately with the new colors. However, there's a potential to simplify the conditional check.
- if (window.globalScope && window.globalScope.renderCanvas) { + if (window.globalScope?.renderCanvas) { setTimeout(() => { window.globalScope.renderCanvas() }, 10) }🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
v1/src/simulator/src/themer/themer.js (1)
108-111: Force canvas redraw to apply theme changes immediatelyAdding the canvas redraw ensures that theme changes are immediately visible to users without requiring a page refresh. This improves the user experience when switching between themes.
Consider using optional chaining for safer object property access:
- if (window.globalScope && window.globalScope.renderCanvas) { + if (window.globalScope?.renderCanvas) { setTimeout(() => window.globalScope.renderCanvas(), 10) }🧰 Tools
🪛 Biome (1.9.4)
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/components/DialogBox/Themes/ApplyThemes.vue(1 hunks)v0/src/simulator/src/themer/customThemeAbstraction.js(1 hunks)v0/src/simulator/src/themer/themer.js(1 hunks)v1/src/simulator/src/themer/customThemeAbstraction.js(1 hunks)v1/src/simulator/src/themer/themer.js(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
v0/src/simulator/src/themer/themer.js
[error] 105-105: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
v1/src/simulator/src/themer/themer.js
[error] 109-109: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (7)
v1/src/simulator/src/themer/customThemeAbstraction.js (1)
36-36: Good addition of--textto the reference listAdding the
--textvariable to therefarray ensures that text color settings properly propagate to all relevant CSS variables, which helps fix the visibility issues mentioned in the PR objectives.v0/src/simulator/src/themer/customThemeAbstraction.js (1)
34-36: LGTM: Improved text color handlingThe changes from
--text-liteto--textas the source property and updating the reference array to include--textand--input-textestablish a more consistent approach to text color handling across the application.src/components/DialogBox/Themes/ApplyThemes.vue (1)
183-221: Well-structured refactoring of custom theme handlingThe restructuring of the
changeCustomThemefunction significantly improves code quality by:
- Caching the DOM element and separating value extraction
- Adding explicit handling for text color properties
- Forcing canvas refresh to ensure changes are visible
This directly addresses the issue of text annotations not changing color appropriately in custom themes.
v0/src/simulator/src/themer/themer.js (2)
87-90: Good simplification of condition check and element accessChanging from
if (selectedTheme === undefined)toif (!selectedTheme)and usingdocument.documentElementinstead ofdocument.getElementsByTagName('html')[0]improves code readability and robustness.
96-99: Great explicit handling for text color in custom themesThis special case handling ensures that the
--textproperty is properly set for custom themes, which directly addresses the core visibility issue this PR aims to fix.v1/src/simulator/src/themer/themer.js (2)
90-93: Code improvement: More concise condition and variable declarationsThe condition has been simplified from explicitly checking for
undefinedto a more general falsy check, which is more concise and handles more edge cases. Additionally, usingdocument.documentElementis a more direct approach thandocument.getElementsByTagName('html')[0].
99-103: Explicitly handle text color in themesThis special handling for the
--textproperty is crucial for ensuring text color visibility across themes, which addresses the core issue mentioned in the PR objectives. By explicitly setting the text color when available, it ensures that text annotations will render with proper contrast against various backgrounds.
| colors['text'] = getComputedStyle( | ||
| document.documentElement | ||
| ).getPropertyValue('--text') |
There was a problem hiding this comment.
Remove duplicate assignment to colors['text']
The colors['text'] property is already assigned at lines 51-53 with the same value. This duplicate assignment should be removed to avoid redundancy and potential confusion.
- colors['text'] = getComputedStyle(
- document.documentElement
- ).getPropertyValue('--text')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| colors['text'] = getComputedStyle( | |
| document.documentElement | |
| ).getPropertyValue('--text') |
|
@JoshVarga please review |
Fixes #518
Fix Text Color Handling in Custom Themes
Description
This PR addresses issues with text color handling in custom themes, particularly focusing on text annotations and their visibility against different backgrounds.
Changes Made
getCanvasColors()Context
Previously, text annotations were not changing color correctly in custom themes despite working properly in automatic themes. This was causing visibility issues when users applied custom themes.
Technical Details
customThemeAbstraction.jsto properly handle text-related CSS variablesthemer.jsto remove redundant color assignments and improve theme applicationTesting
To test these changes:
Related
CircuitVerse/CircuitVerse#4460 (comment)
Fixes #5487
Type of Change
Screenshots (if appropriate)
Screen.Recording.2025-03-10.at.3.38.27.AM.mov