feat(site): switch theme styles through routing #2574
Conversation
WalkthroughThe changes in this pull request focus on enhancing the theme management logic within the Changes
Possibly related PRs
Suggested labels
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![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 (5)
examples/sites/src/tools/useTheme.js (3)
6-6: Consider lazy loading theme assets for better performance.The theme-related images are imported statically, which could impact the initial bundle size. Consider lazy loading these assets when they're needed, especially since theme switching is a dynamic operation.
Example approach:
- import glaciers from '@/assets/images/glaciers.png' - import glaciersIcon from '@/assets/images/glaciers-icon.png' + const loadThemeAssets = async (theme) => { + const glaciers = await import('@/assets/images/glaciers.png') + const glaciersIcon = await import('@/assets/images/glaciers-icon.png') + return { image: glaciers.default, icon: glaciersIcon.default } + }Also applies to: 18-18
Line range hint
90-103: Add error handling and support theme switching.The current implementation has several potential issues:
- The
loadedThemeflag prevents reloading the old theme if users switch back to it after changing to another theme- Missing error handling for theme tool initialization
- No cleanup of watchers on component unmount
Consider this improved implementation:
- let loadedTheme = false + let currentLoadedTheme = null const watchRoute = () => { if (initWatchRoute) { return } initWatchRoute = true - watch( + const stopWatch = watch( () => router.currentRoute.value.params.theme, (val) => { - if (!loadedTheme && val === 'old-theme') { + if (val === 'old-theme' && currentLoadedTheme !== 'old-theme') { const themeTool = new TinyThemeTool() + try { themeTool.changeTheme(tinyOldTheme) - loadedTheme = true + currentLoadedTheme = 'old-theme' + } catch (error) { + console.error('Failed to load old theme:', error) + } + } else if (val !== 'old-theme') { + currentLoadedTheme = val } } ) + // Cleanup watcher when component unmounts + if (import.meta.hot) { + import.meta.hot.dispose(() => { + stopWatch() + }) + } }
Line range hint
109-116: Add input validation and documentation for the theme hook.The theme switching implementation could benefit from additional safety checks and documentation.
Consider these improvements:
+ /** + * Hook for managing theme switching functionality + * @returns {Object} Theme management utilities + * @property {Function} getThemeData - Returns copy of available theme data + * @property {Function} changeTheme - Switches to specified theme + * @property {Ref<string>} currentThemeKey - Current theme key + * @property {Object} designConfig - Current theme design configuration + * @property {string} defaultTheme - Default theme route value + */ export default function useTheme() { !initWatchRoute && watchRoute() + const validateTheme = (themeKey) => { + if (!THEME_ROUTE_MAP[themeKey]) { + console.warn(`Invalid theme key: ${themeKey}`) + return false + } + return true + } + + const safeChangeTheme = (themeKey) => { + if (validateTheme(themeKey)) { + changeTheme(themeKey) + } + } return { getThemeData, - changeTheme, + changeTheme: safeChangeTheme, currentThemeKey, designConfig, defaultTheme: THEME_ROUTE_MAP[defaultThemeKey] } }packages/theme/src/old-theme-index.js (2)
5-5: Consider documenting typography scale ratios.The typography system uses fixed pixel values. Consider adding documentation about the scale ratio between different sizes (sm, md, lg, xl, xxl) to maintain consistency when updates are needed.
5-5: Consider extracting Button variables to a separate component theme file.Currently, Button-specific variables are embedded in the main theme file. As more components are added, this approach might not scale well.
Consider:
- Creating a separate file for component-specific theme variables
- Implementing a modular theme system where components can define their theme variables
- Using a theme composition pattern to merge component themes with the base theme
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
examples/sites/src/tools/useTheme.js(4 hunks)packages/theme/src/old-theme-index.js(1 hunks)
🔇 Additional comments (4)
examples/sites/src/tools/useTheme.js (1)
Line range hint 90-116: Add test coverage for theme switching logic.
To ensure the reliability of the theme switching functionality, consider adding tests for:
- Route parameter changes triggering correct theme changes
- Theme validation
- Error handling during theme tool initialization
Let's check for existing test files:
Would you like me to help create a test suite for this functionality?
packages/theme/src/old-theme-index.js (3)
Line range hint 1-4: LGTM! Clear theme identification.
The theme is well-identified with both English and Chinese names, following a consistent naming pattern.
5-5: Verify color contrast ratios for accessibility.
The color system defines multiple shades for each color type (brand, success, error, warn, info). While the color palette is comprehensive, it's important to ensure that text-background combinations meet WCAG accessibility guidelines.
5-5: Verify shadow values for consistency across components.
The shadow system defines multiple levels (0-4) with different directions (up, down, left, right). Ensure these values provide consistent depth perception across different components.
✅ Verification successful
Shadow system is well-defined and consistent
The theme defines a comprehensive and consistent shadow system with 5 levels (0-4) and 4 directional variants (up, down, left, right) for each level. The values show a clear progression:
- Level 0: Base shadow (0 4px 16px 0)
- Level 1: Subtle shadow (0/±1px 4px 0)
- Level 2: Medium shadow (0/±2px 8px 0)
- Level 3: Pronounced shadow (0/±4px 16px 0)
- Level 4: Strong shadow (0/±8px 40px 0)
Each level consistently doubles the offset and increases the blur radius, while maintaining appropriate opacity values (0.08 or 0.16) for depth perception.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for shadow usage across components to verify consistency
# Look for shadow variable usage
rg --type js --type vue "shadow-[0-4]-(up|down|left|right)" -A 2
Length of output: 94
Script:
#!/bin/bash
# Search for shadow CSS variable usage with ripgrep
rg --type js "shadow-[0-4]|--tv-shadow-[0-4]" -A 2
# Also search for shadow properties in CSS/SCSS files
rg --type css "shadow-[0-4]|--tv-shadow-[0-4]" -A 2
Length of output: 17331
Script:
#!/bin/bash
# Search for actual usage of shadow variables in component files
rg "var\(--tv-shadow-[0-4]" --type js --type css --type vue -A 1
# Search for box-shadow property usage
rg "box-shadow:" --type js --type css --type vue -A 1
Length of output: 176
PR
PR Checklist
Please check if your PR fulfills the following requirements:
Other information
通过路由切换old-theme
1、每次修改旧主题，需要执行 pnpm build:theme, 以便重新生成old-theme.css
2、在路径上切换 old-theme即可。
Summary by CodeRabbit
New Features
Bug Fixes
Style