refactor : Ts integration : src/simulator/src/themer/themer.ts #463
Conversation
WalkthroughThis pull request migrates the theme management functionality in the simulator application from JavaScript to TypeScript. The deprecated JavaScript module (themer.js) is removed, and a new TypeScript module (themer.ts) is introduced with equivalent functions to handle canvas color extraction, CSS variable updates, dynamic SVG generation, theme card HTML creation, and dialog management for theme selection. Additionally, new interfaces (ThemeOptions, Themes, and Theme) are added to support flexible theme configurations. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TD as Theme Dialog
participant TM as Theme Module
participant LS as Local Storage
participant CSS as CSS Variables
U->>TD: Open theme selection dialog
TD->>TM: Trigger colorThemes()
TM->>LS: Check for stored theme
LS-->>TM: Return current/default theme
TM->>CSS: updateThemeForStyle(themeName)
CSS-->>TM: Apply new CSS variables
TM->>TM: updateBG() to refresh background
TM-->>TD: Return updated UI
U->>TD: View updated theme
Assessment against linked issues
Poem
✨ Finishing Touches
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 (
|
✅ Deploy Preview for circuitverse ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ 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: 3
🧹 Nitpick comments (6)
src/simulator/src/themer/themer.types.ts (1)
1-5: Consider adding more specific type definitions.While the current interface is flexible, consider adding more specific types to improve type safety and documentation:
+// Define specific CSS property names +type CSSPropertyName = '--text-panel' | '--bg-tab' | '--br-primary' | /* other properties */; + +// Define specific CSS property values +type CSSPropertyValue = string; + +// Define specific theme names +type ThemeName = 'Default Theme' | 'Dark Theme' | /* other themes */; + export interface ThemeOptions { - [key: string]: { - [property: string]: string; - }; + [key in ThemeName]: { + [property in CSSPropertyName]: CSSPropertyValue; + }; }src/simulator/src/themer/themer.ts (5)
1-8: Organize imports for better readability.Consider organizing imports into groups:
- External dependencies
- Internal modules
- Types
+// External dependencies import { dots } from '../canvasApi'; +import { SimulatorStore } from '#/store/SimulatorStore/SimulatorStore'; + +// Internal modules import importedThemeOptions from './themes'; +import themeCardSvg from './themeCardSvg'; + +// Types import{ ThemeOptions } from './themer.types' - const themeOptions: ThemeOptions = importedThemeOptions; -import themeCardSvg from './themeCardSvg'; -import { SimulatorStore } from '#/store/SimulatorStore/SimulatorStore';
13-76: Refactor repetitive code for better maintainability.The function contains repetitive code for getting computed styles. Consider extracting the common logic into a helper function.
+const getCSSVariable = (variableName: string): string => { + return getComputedStyle(document.documentElement).getPropertyValue(variableName); +}; + const getCanvasColors = (): Record<string, string> => { const colors: Record<string, string> = {}; - colors['hover_select'] = getComputedStyle( - document.documentElement - ).getPropertyValue('--hover-and-sel'); - colors['fill'] = getComputedStyle( - document.documentElement - ).getPropertyValue('--fill'); + colors['hover_select'] = getCSSVariable('--hover-and-sel'); + colors['fill'] = getCSSVariable('--fill'); // ... similar refactoring for other properties return colors; };
89-97: Consider using a more functional approach.The function mutates a global
colorsvariable, which could lead to unexpected behavior. Consider using a more functional approach.-export let colors: Record<string, string> = getCanvasColors(); +export const getColors = (): Record<string, string> => { + return Object.freeze(getCanvasColors()); +}; + +let currentColors: Record<string, string> = getColors(); + export function updateThemeForStyle(themeName: string): void { const selectedTheme = themeOptions[themeName]; if (selectedTheme === undefined) return; const html = document.getElementsByTagName('html')[0]; Object.keys(selectedTheme).forEach((property) => { html.style.setProperty(property, selectedTheme[property]); }); - colors = getCanvasColors(); + currentColors = getColors(); } + +export const getCurrentColors = (): Record<string, string> => currentColors;
227-248: Improve event handling and type safety.The event handling code could be improved with better type safety and error handling.
- document.querySelectorAll('.themeSel').forEach((element) => { + document.querySelectorAll<HTMLElement>('.themeSel').forEach((element) => { element.addEventListener('mousedown', (e) => { e.preventDefault(); - document.querySelectorAll('.selected').forEach((el) => { + document.querySelectorAll<HTMLElement>('.selected').forEach((el) => { el.classList.remove('selected'); }); const themeCard = (e.target as HTMLElement).parentElement; - if (themeCard) { + if (themeCard instanceof HTMLElement) { themeCard.classList.add('selected'); const radioButton = themeCard.querySelector('input[type=radio]') as HTMLInputElement; - if (radioButton) { + if (radioButton instanceof HTMLInputElement) { radioButton.click(); // Mark as selected } const label = themeCard.querySelector('label'); - if (label) { + if (label instanceof HTMLLabelElement) { updateThemeForStyle(label.textContent || ''); // Extract theme name and set + } else { + console.error('Label element not found'); } updateBG(); + } else { + console.error('Theme card element not found'); } }); });
253-258: Add error handling for localStorage operations.The initialization code should handle cases where localStorage is not available.
(() => { + try { if (!localStorage.getItem('theme')) { localStorage.setItem('theme', 'Default Theme'); } updateThemeForStyle(localStorage.getItem('theme') || 'Default Theme'); + } catch (error) { + console.error('Failed to initialize theme:', error); + updateThemeForStyle('Default Theme'); + } })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulator/src/themer/themer.js(0 hunks)src/simulator/src/themer/themer.ts(1 hunks)src/simulator/src/themer/themer.types.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/simulator/src/themer/themer.js
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - circuitverse
- GitHub Check: Header rules - circuitverse
- GitHub Check: Pages changed - circuitverse
🔇 Additional comments (1)
src/simulator/src/themer/themer.ts (1)
250-250: LGTM!The function is simple and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/simulator/src/themer/themer.ts (5)
3-3: Fix import statement formatting.There's a missing space after the opening brace.
-import{ ThemeOptions } from './themer.types' +import { ThemeOptions } from './themer.types'
13-76: Reduce code duplication in color extraction.The function has repetitive getComputedStyle calls that can be simplified using a mapping object and a loop.
const getCanvasColors = (): Record<string, string> => { + const colorMapping = { + 'hover_select': '--hover-and-sel', + 'fill': '--fill', + 'mini_fill': '--mini-map', + 'mini_stroke': '--mini-map-stroke', + // ... add other mappings + }; + const colors: Record<string, string> = {}; - colors['hover_select'] = getComputedStyle( - document.documentElement - ).getPropertyValue('--hover-and-sel'); - // ... other repeated calls + Object.entries(colorMapping).forEach(([key, cssVar]) => { + colors[key] = getComputedStyle( + document.documentElement + ).getPropertyValue(cssVar); + }); return colors; };
89-97: Add error logging for undefined themes.The function silently returns when a theme is undefined. Consider logging an error to help with debugging.
export function updateThemeForStyle(themeName: string): void { const selectedTheme = themeOptions[themeName]; - if (selectedTheme === undefined) return; + if (selectedTheme === undefined) { + console.error(`Theme "${themeName}" not found`); + return; + } const html = document.getElementsByTagName('html')[0];
105-150: Add type safety for SVG element manipulation.The SVG manipulation could benefit from stronger type safety using TypeScript's SVG types.
- svgElement.querySelectorAll('.svgText').forEach((el) => { + svgElement.querySelectorAll('.svgText').forEach((el: SVGElement) => { el.setAttribute('fill', colors['--text-panel'] || '#000000'); });
228-233: Add error handling for localStorage access.The initialization code should handle cases where localStorage is not available (e.g., in private browsing mode).
(() => { + try { if (!localStorage.getItem('theme')) { localStorage.setItem('theme', 'Default Theme'); } updateThemeForStyle(localStorage.getItem('theme') || 'Default Theme'); + } catch (error) { + console.error('Failed to access localStorage:', error); + updateThemeForStyle('Default Theme'); + } })();
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/simulator/src/themer/themer.ts (4)
1-8: Organize imports and add missing type annotation.Consider organizing imports by type (external, internal, types) and adding type annotation for importedThemeOptions.
+// External imports import { dots } from '../canvasApi'; -import importedThemeOptions from './themes'; -import{ ThemeOptions } from './themer.types' +import { SimulatorStore } from '#/store/SimulatorStore/SimulatorStore'; -const themeOptions: ThemeOptions = importedThemeOptions; +// Internal imports +import importedThemeOptions from './themes'; import themeCardSvg from './themeCardSvg'; -import { SimulatorStore } from '#/store/SimulatorStore/SimulatorStore'; + +// Types +import { ThemeOptions } from './themer.types' + +const themeOptions: ThemeOptions = importedThemeOptions as ThemeOptions;
13-76: Reduce code duplication in getCanvasColors.The function has repetitive calls to getComputedStyle. Consider refactoring to a more maintainable structure.
const getCanvasColors = (): Record<string, string> => { + const getProperty = (name: string): string => + getComputedStyle(document.documentElement).getPropertyValue(name); + + const colorMap: [string, string][] = [ + ['hover_select', '--hover-and-sel'], + ['fill', '--fill'], + ['mini_fill', '--mini-map'], + // ... other mappings + ]; + const colors: Record<string, string> = {}; - colors['hover_select'] = getComputedStyle( - document.documentElement - ).getPropertyValue('--hover-and-sel'); - // ... other properties + colorMap.forEach(([key, cssVar]) => { + colors[key] = getProperty(cssVar); + }); return colors; };
89-97: Add error logging and improve type safety.The function silently returns when the theme is undefined. Consider adding error logging and improving type safety.
export function updateThemeForStyle(themeName: string): void { const selectedTheme = themeOptions[themeName]; - if (selectedTheme === undefined) return; + if (selectedTheme === undefined) { + console.error(`Theme "${themeName}" not found`); + return; + } + const html = document.getElementsByTagName('html')[0]; + if (!html) { + console.error('HTML element not found'); + return; + } + Object.keys(selectedTheme).forEach((property) => { html.style.setProperty(property, selectedTheme[property]); }); colors = getCanvasColors(); }
241-247: Add error handling for localStorage operations.The initialization code should handle cases where localStorage is not available.
// Initialize theme on load (() => { + try { + const storedTheme = localStorage.getItem('theme'); + const defaultTheme = 'Default Theme'; + + if (!storedTheme) { + localStorage.setItem('theme', defaultTheme); + } + + updateThemeForStyle(storedTheme || defaultTheme); + } catch (error) { + console.error('Failed to access localStorage:', error); + updateThemeForStyle('Default Theme'); + } - if (!localStorage.getItem('theme')) { - localStorage.setItem('theme', 'Default Theme'); - } - updateThemeForStyle(localStorage.getItem('theme') || 'Default Theme'); })();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/simulator/src/themer/themer.ts(1 hunks)src/simulator/src/themer/themer.types.ts(1 hunks)
🔇 Additional comments (4)
src/simulator/src/themer/themer.types.ts (1)
1-13: LGTM! Well-structured type definitions.The interfaces provide a good balance between type safety and flexibility for theme configurations. The use of index signatures allows for dynamic theme properties while maintaining type checking.
src/simulator/src/themer/themer.ts (3)
105-150: Previous review comments already addressed the improvements needed.
167-182: Previous review comments already addressed the improvements needed.
187-237: Previous review comments already addressed the improvements needed.
|
@JoshVarga @vedant-jain03 Also tell me if i should keep themer.ts or should i convert it to vue PS: Since this file |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/simulator/src/themer/themer.ts (1)
113-129:⚠️ Potential issueAdd HTML escaping to prevent XSS attacks.
The function should escape HTML special characters in user-provided values.
🧹 Nitpick comments (5)
src/simulator/src/themer/themer.ts (5)
12-14: Add error handling for missing CSS variables.The
setColorfunction should handle cases where CSS variables are not defined.const setColor = (colors: Record<string, string>, key: string, cssVar: string): void => { - colors[key] = getComputedStyle(document.documentElement).getPropertyValue(cssVar); + const value = getComputedStyle(document.documentElement).getPropertyValue(cssVar); + colors[key] = value || '#000000'; // Provide a default color if CSS variable is not defined };
47-47: Consider using a reactive state management solution.The mutable global
colorsvariable could lead to race conditions in concurrent updates. Consider using a reactive state management solution or making it immutable.
52-62: Enhance error handling with user feedback.The function silently returns when the theme is undefined. Consider providing user feedback and logging the error.
export function updateThemeForStyle(themeName: string): void { const selectedTheme = themeOptions[themeName]; - if (selectedTheme === undefined) return; + if (selectedTheme === undefined) { + console.error(`Theme "${themeName}" not found`); + // Optionally show user feedback via UI + return; + }
153-154: Improve type safety in event handling.Instead of using type assertion, consider using proper event typing.
- element.addEventListener('mousedown', mousedownHandler as EventListener); - cleanupListeners.push(() => element.removeEventListener('mousedown', mousedownHandler as EventListener)); + element.addEventListener('mousedown', mousedownHandler); + cleanupListeners.push(() => element.removeEventListener('mousedown', mousedownHandler));
194-200: Add error handling for localStorage access.The function should handle cases where localStorage is not available or throws errors.
const initializeTheme = (): void => { - const theme = localStorage.getItem('theme') || 'Default Theme'; - if (!localStorage.getItem('theme')) { - localStorage.setItem('theme', theme); + let theme = 'Default Theme'; + try { + const storedTheme = localStorage.getItem('theme'); + if (storedTheme) { + theme = storedTheme; + } else { + localStorage.setItem('theme', theme); + } + } catch (error) { + console.error('Failed to access localStorage:', error); } updateThemeForStyle(theme); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/themer/themer.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - circuitverse
- GitHub Check: Header rules - circuitverse
- GitHub Check: Pages changed - circuitverse
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
src/simulator/src/themer/themer.ts (2)
1-8: LGTM! Clean imports and initialization.The imports are well-organized with proper type separation, and the theme options initialization is clear.
76-108: LGTM! Well-structured SVG generation with proper error handling.The code includes comprehensive error handling for theme validation and SVG parsing, with clean SVG manipulation using helper functions.
|
addressing further code climate issues to decrease code length will increase Cognitive complexity which is not favorable |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/simulator/src/themer/themer.ts (1)
113-129:⚠️ Potential issueAdd HTML escaping to prevent XSS attacks.
The function should escape HTML special characters in user-provided values.
🧹 Nitpick comments (5)
src/simulator/src/themer/themer.ts (5)
12-14: Add error handling for getComputedStyle.The setColor function should handle potential errors when accessing CSS properties.
const setColor = (colors: Record<string, string>, key: string, cssVar: string): void => { - colors[key] = getComputedStyle(document.documentElement).getPropertyValue(cssVar); + try { + const value = getComputedStyle(document.documentElement).getPropertyValue(cssVar); + colors[key] = value || '#000000'; // Fallback color if empty + } catch (error) { + console.error(`Failed to get CSS variable ${cssVar}:`, error); + colors[key] = '#000000'; // Fallback color on error + } };
47-47: Consider making colors readonly.The exported colors variable is mutable, which could lead to unintended modifications.
-export let colors: Record<string, string> = getCanvasColors(); +export const colors: Readonly<Record<string, string>> = getCanvasColors();
52-62: Enhance error handling in updateThemeForStyle.The function should provide better error feedback when a theme is not found.
export function updateThemeForStyle(themeName: string): void { const selectedTheme = themeOptions[themeName]; - if (selectedTheme === undefined) return; + if (selectedTheme === undefined) { + console.error(`Theme "${themeName}" not found in available themes`); + return; + } const html = document.documentElement; Object.keys(selectedTheme).forEach((property) => { - html.style.setProperty(property, selectedTheme[property]); + try { + html.style.setProperty(property, selectedTheme[property]); + } catch (error) { + console.error(`Failed to set CSS property ${property}:`, error); + } }); colors = getCanvasColors(); }
134-159: Improve type safety in handleThemeSelection.The function uses type assertions that could be made safer.
- const themeCard = (e.target as HTMLElement).parentElement; + const target = e.target; + if (!(target instanceof HTMLElement)) return; + const themeCard = target.parentElement; if (!themeCard) return;
209-215: Validate stored theme before use.The function should validate that the stored theme exists in themeOptions.
const initializeTheme = (): void => { - const theme = localStorage.getItem('theme') || 'Default Theme'; + let theme = localStorage.getItem('theme') || 'Default Theme'; + if (!themeOptions[theme]) { + console.warn(`Stored theme "${theme}" not found, falling back to Default Theme`); + theme = 'Default Theme'; + } if (!localStorage.getItem('theme')) { localStorage.setItem('theme', theme); } updateThemeForStyle(theme); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/simulator/src/themer/themer.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
src/simulator/src/themer/themer.ts (3)
1-8: LGTM! Clean imports and initialization.The imports are well-organized with proper type separation.
76-108: LGTM! Proper error handling implemented.The function includes robust error handling for theme validation and SVG parsing as suggested in the previous review.
176-199: LGTM! Proper event listener cleanup implemented.The function includes proper cleanup of event listeners as suggested in the previous review.
Fixes #414
@JoshVarga @vedant-jain03
Kindly have a look
Summary by CodeRabbit
New Features
Changes