feat：add theme switching plugin #1070
Conversation
WalkthroughThis pull request updates the theme configuration across the project by replacing dynamic environment variable checks with a static Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (6)
✨ 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 (
|
![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 (15)
packages/toolbars/theme/meta.js (1)
1-12: Consider adding doc comments for metadata fields.
Adding explanatory doc comments (e.g., describingid,type,title, and each option) can improve maintainability and clarity for future contributors.packages/toolbars/theme/index.js (1)
17-20: Renameentryfor clarity.
Consider renaming theentryproperty to something likethemeEntryormainEntryto improve readability.export default { ...metaData, - entry + themeEntry: entry }packages/toolbars/theme/vite.config.js (1)
13-35: Consider supporting more build formats.
The build config is straightforward. If supporting older environments or alternative module systems is needed, consider adding a UMD or CommonJS output.build: { lib: { entry: path.resolve(__dirname, './index.js'), name: 'toolbar-theme', fileName: () => 'index.js', - formats: ['es'] + formats: ['es', 'cjs'] }, rollupOptions: { external: ['vue', /@opentiny\/tiny-engine.*/, /@opentiny\/vue.*/] } }packages/toolbars/theme/src/Main.vue (4)
1-9: Consider gracefully handling empty or null options.
Whenoptionsis empty or not provided, thetoolbar-baseprops might still be passed in. Consider conditionally disabling the toolbar or providing a fallback UI to avoid confusion.
10-21: Extract display text (“主题”) for i18n flexibility.
Hardcoding text in Chinese might limit multilingual support. Consider passing a prop for the radio group title or integrating a localization plugin to keep it consistent with the rest of the application.
36-45: Validate the range of valid positions.
Thepositionprop may accept both'right'and'collapse'. If these are the only valid values, consider using an explicit validator or an enum-style approach to enhance reliability and maintainability.
46-55: Reuse the composable instance.
You calluseTheme()twice, once forinitThemeState()and again forthemeChange. For improved clarity, store the result in a variable and destructure both functions from a single call:setup() { - const state = useTheme().initThemeState() - const themeChange = useTheme().themeChange + const theme = useTheme() + const state = theme.initThemeState() + const { themeChange } = theme return { state, themeChange } }packages/common/component/ToolbarBase.vue (3)
9-12: Avoid empty spans for spacing.
The empty<span>on line 11 might be purely for spacing. Prefer styling or margin to maintain layout, removing extra DOM nodes.
37-40: Prop definition forpositionis clear.
It correctly defaults to'right'. A validator or enumerated options might further clarify acceptable values, but this is otherwise fine.
59-68: Returnnullinstead offalsefor clarity.
Within Vue, usingfalseto disable rendering is valid, butnullis more explicit and makes the code slightly more readable. Consider returningnullingetRender()as a minor improvement.- return false + return nullpackages/toolbars/theme/src/composable/useTheme.js (1)
47-60: Toggling logic is clear; consider edge cases.
The fallback toggling between light/dark covers two themes. If additional themes are introduced later, ensure this logic is extended accordingly. Otherwise, this approach is straightforward.packages/engine-cli/template/designer/registry.js (1)
91-91: Placement of theThemecomponent intoolbarsarray.The
Themecomponent is placed first in the array; verify this ordering is intentional. If it should appear next to items likeLangorViewSetting, consider reordering for a consistent user flow.designer-demo/registry.js (2)
68-68: Add'engine.toolbars.theme'to therightarray.Placing
'engine.toolbars.theme'in therightarray is consistent with the new theming feature. Ensure the icon or label is intuitive for easy user discovery.
76-77: Avoid potential confusion when repeating the same toolbar item.You've inserted
'engine.toolbars.theme'in both therightandcollapsearrays. If this is intentional to make the Theme toolbar prominently accessible, that's fine, but be aware of possible UI duplication.packages/design-core/src/init.js (1)
58-58: LeveragelocalStoragefallback for theme selection.Using
localStorage.getItem('tiny-engine-theme')ensures user preferences persist. Consider validating the stored value to avoid unexpected states if localStorage data is corrupted or invalid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
packages/design-core/assets/cn.svgis excluded by!**/*.svgpackages/design-core/assets/dark.svgis excluded by!**/*.svgpackages/design-core/assets/en.svgis excluded by!**/*.svgpackages/design-core/assets/light.svgis excluded by!**/*.svgpackages/design-core/assets/nested-page.svgis excluded by!**/*.svgpackages/design-core/assets/single-page.svgis excluded by!**/*.svg
📒 Files selected for processing (22)
designer-demo/engine.config.js(1 hunks)designer-demo/registry.js(3 hunks)jsconfig.json(2 hunks)packages/build/vite-config/src/vite-plugins/devAliasPlugin.js(1 hunks)packages/common/component/ToolbarBase.vue(3 hunks)packages/design-core/index.js(1 hunks)packages/design-core/package.json(1 hunks)packages/design-core/src/init.js(2 hunks)packages/engine-cli/src/commands/generateConfig.js(1 hunks)packages/engine-cli/template/designer/engine.config.js(1 hunks)packages/engine-cli/template/designer/registry.js(3 hunks)packages/layout/index.js(1 hunks)packages/layout/src/DesignToolbars.vue(1 hunks)packages/layout/src/ToolbarCollapse.vue(2 hunks)packages/register/src/constants.js(1 hunks)packages/toolbars/theme/index.js(1 hunks)packages/toolbars/theme/meta.js(1 hunks)packages/toolbars/theme/package.json(1 hunks)packages/toolbars/theme/src/Main.vue(1 hunks)packages/toolbars/theme/src/composable/useTheme.js(1 hunks)packages/toolbars/theme/src/styles/vars.less(1 hunks)packages/toolbars/theme/vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- designer-demo/engine.config.js
- packages/toolbars/theme/src/styles/vars.less
- packages/engine-cli/template/designer/engine.config.js
- packages/toolbars/theme/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (29)
packages/engine-cli/src/commands/generateConfig.js (1)
11-11: Verify if'${theme}'is intended as a literal placeholder or dynamic interpolation.Currently,
'${theme}'will be rendered as a plain string in this generated code. If your goal is to inject the actualthemevalue, consider using JavaScript template interpolation (e.g.,theme: \`${theme}\`) or confirm that a build-time replacement step is substituting'${theme}'with the desired value. Otherwise,'${theme}'may remain as a literal placeholder and potentially cause confusion or broken theme logic in the resulting configuration file.packages/toolbars/theme/meta.js (1)
1-12: Overall implementation looks solid.
No issues with object structure or property naming. This aligns well with the existing conventions in the project.packages/toolbars/theme/index.js (1)
1-11: License header check.
The license and copyright statements appear correctly formatted and consistent with the project.packages/layout/index.js (2)
19-19: Theme integration in right toolbar.
Addingengine.toolbars.themehelps ensure the user can quickly switch themes. Looks good.
28-28: Theme integration in collapse toolbar.
Providing the theme switch in the collapsed section is also helpful. No conflicts detected.packages/toolbars/theme/vite.config.js (1)
1-11: License header confirmed.
Everything is consistent with the project’s license disclaimers.packages/toolbars/theme/src/Main.vue (2)
26-35: Script and component imports look good!
The chosen imports and components appear consistent with the overall codebase style.
58-65: Scoped styles are well-scoped and consistent with theme.
No issues found here; using a CSS variable encourages theme consistency.packages/common/component/ToolbarBase.vue (2)
55-57: Helper methods cleanly separate logic.
TheisShowMultiandisHideMultimethods provide clarity and readability for controlling collapsible states.
74-76: Final exports are neatly organized.
ExposinggetRender,isShowMulti, andisHideMultifrom the setup function is straightforward and helps reactivity.packages/register/src/constants.js (1)
29-29: NewThemeconstant aligns well with existing naming.
This addition fits neatly among the other engine toolbar constants.packages/toolbars/theme/src/composable/useTheme.js (3)
17-22:THEME_DATAstructure is concise and clear.
Defining both name and label strings here keeps them well-organized for easy reference.
39-45: Handle invalid or unexpected theme values.
Currently,initThemeStatedefaults to'light'if no stored theme is found. You may want to validate the stored or merged theme in case of third-party modifications or storage errors.
62-69: Neat export of composable functionality.
The composable returns all relevant objects and methods for theme management, keeping the code modular and maintainable.packages/engine-cli/template/designer/registry.js (3)
24-24: Add import statement forTheme.Nice addition of the
Themeimport from@opentiny/tiny-engine. This aligns well with the introduction of new theming capabilities.
68-68: Ensure toolbar references match existing aliases.You added
'engine.toolbars.theme'to the right toolbar array. Make sure'engine.toolbars.theme'is properly registered in the global config so it doesn't cause runtime errors.
76-77: Confirm duplication in collapsed toolbar.The
'engine.toolbars.theme'entry also appears in the collapsed toolbar. Confirm whether you intend to display the same toolbar item in two places or if this is unintended duplication.designer-demo/registry.js (2)
24-24: Add import statement forTheme.Importing
Themefrom@opentiny/tiny-engineimproves modularity and allows seamless integration of the theming functionality across the demo.
91-91: IncludeThemein the maintoolbarsarray.Co-locating
Themewith other entries ensures feature parity across various deployment scenarios. No major concerns here; just verify consistent ordering with the rest of the toolbars.packages/design-core/index.js (1)
14-14: ExportThemefor modular usage.Exporting
Themefrom@opentiny/tiny-engine-toolbar-themekeeps the design-core consistent and extensible. Good job on ensuring this is easily importable elsewhere.packages/design-core/src/init.js (1)
75-77: Set theme-based editor mode.Fetching
themefromlocalStoragein lines 75–77 is a smooth way to unify UI and editor theme. However, confirm that user-defined theme strings (like custom theme names) won't break thedarkcheck. Consider a more robust approach if you plan to add advanced theming options in the future.packages/layout/src/DesignToolbars.vue (2)
23-27: Ensurepositionprop is documented and handled.
The addition ofposition="right"here mirrors the usage in other components. Confirm that the component receiving this prop properly handles or documentsposition, so behavior is consistent across toolbars.
31-35: Maintain consistency for props in array-of-components scenario.
Applyingposition="right"to each nested component is a good approach, so that items are consistently displayed in the right toolbar. This appears correct and in line with the rest of the changes.packages/build/vite-config/src/vite-plugins/devAliasPlugin.js (1)
50-50: Validate new theme alias.
The alias@opentiny/tiny-engine-toolbar-themeis added correctly. Ensure that references elsewhere in the codebase match this alias to avoid resolution issues.✅ Verification successful
Theme alias configuration is consistent ✅
The alias@opentiny/tiny-engine-toolbar-themeis properly configured in all relevant files and used consistently across the codebase. The package is correctly referenced in dependencies and imports.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verifies usage of the new alias in codebase rg '@opentiny/tiny-engine-toolbar-theme'Length of output: 670
packages/layout/src/ToolbarCollapse.vue (2)
2-2: Confirm popover width change.
Changing the popover width from140to248might impact overall toolbar layout. Ensure there is enough space and that the new width does not cause overflow in smaller viewports.
19-23: Consistent usage ofcompin array loop.
Here,compis used correctly for items in an array. Make sure it remains consistent if the references are renamed in the block above.jsconfig.json (2)
34-34: Added theme toolbar path alias.
The new path mapping for@opentiny/tiny-engine-toolbar-themeensures easy imports of the theme entry point. It appears correctly aligned with the directory structure.
63-63: Wildcard alias for theme toolbar.
This wildcard alias supports imports of submodules within@opentiny/tiny-engine-toolbar-theme/*. It aligns well with the newly introduced theming architecture.packages/design-core/package.json (1)
75-75: LGTM! The new theme toolbar dependency is properly added.The dependency is correctly positioned among other toolbar-related dependencies and uses the consistent
workspace:*version pattern.Let's verify the integration with existing theme packages:
✅ Verification successful
Theme toolbar integration verified successfully
The new theme toolbar package is properly integrated with the existing theme system and follows the established patterns:
- Correctly registered in toolbar configurations alongside other theme components
- Properly implemented with theme switching UI and styling
- Well integrated with the build system and package structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential conflicts or integration issues between the new theme toolbar and existing theme packages # Search for theme-related imports and usage echo "Checking theme-related imports and usage patterns:" rg -A 3 "import.*theme|from.*theme" --type vue --type js # Check for potential conflicts in theme configuration echo "Checking theme configuration patterns:" rg -A 3 "theme.*config|configureTheme|themeConfig" --type vue --type js # Look for existing toolbar registrations to ensure proper integration echo "Checking toolbar registration patterns:" rg -A 3 "registerToolbar|toolbar.*config" --type vue --type jsLength of output: 568
Script:
#!/bin/bash # Search for theme-related imports and configurations echo "=== Theme-related patterns ===" rg "from ['\"]\@opentiny/tiny-engine-theme" -A 2 echo -e "\n=== Theme configuration patterns ===" rg "themeConfig|configureTheme" -A 2 echo -e "\n=== Toolbar registration and theme integration ===" rg "registerToolbar|toolbarConfig|themeToolbar" -A 2 # Use ast-grep to find theme-related function calls and configurations echo -e "\n=== Theme-related function calls ===" ast-grep --pattern 'registerTheme($_)' echo -e "\n=== Theme configuration objects ===" ast-grep --pattern 'themeConfig = { $$$ }'Length of output: 800
Script:
#!/bin/bash # Check for imports and usage of the new theme toolbar package echo "=== New theme toolbar package usage ===" rg "@opentiny/tiny-engine-toolbar-theme" -A 2 # Search for theme-related imports with fixed regex echo -e "\n=== Theme-related imports ===" rg "from.*@opentiny/tiny-engine-theme" -A 2 # Look for theme and toolbar related patterns echo -e "\n=== Theme and toolbar patterns ===" rg "theme.*toolbar|toolbar.*theme" -A 2 # Check package.json files for related dependencies echo -e "\n=== Related dependencies in package.json files ===" fd "package.json" --exec grep -A 3 -B 3 "tiny-engine.*theme"Length of output: 9389
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/toolbars/theme/src/composable/useTheme.js (2)
39-47: Local storage usage and fallback.
Initializing theme state fromlocalStoragewith fallback toengine.config.themeis robust. The function returns the reactive state as expected.Consider verifying that localStorage is available in all environments where this code may run (for instance, server-side rendering or embedded frames).
49-63: Smart toggling and integration with editor theming.
The logic to determine whether to toggle or directly set the theme is intuitive. The default usage of'vs-dark'vs'vs'for the Monaco editor is also consistent.If more themes are introduced in the future, consider expanding the logic beyond a simple
'includes("dark")'check for better scalability.packages/design-core/src/init.js (1)
78-81: Potential duplication in retrieving app ID and theme.
You’ve duplicated the logic to retrievethemefrom localStorage in bothbeforeAppCreateandappCreated.Extracting a helper function or using the newly defined composable in future refactors could reduce repetition and help maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/design-core/src/init.js(4 hunks)packages/toolbars/theme/src/composable/useTheme.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (7)
packages/toolbars/theme/src/composable/useTheme.js (5)
1-11: Header looks good.
These license and copyright notices appear consistent and comprehensive.
13-16: Imports correctly reference necessary APIs.
All imported functions (reactive,getMetaApi,getMergeMeta,META_SERVICE,setGlobalMonacoEditorTheme) are used. Everything looks fine.
17-22: Naming alignment for theme constants.
The theme constants (LIGHT,DARK) and their matching labels are well-structured. No immediate issues.
24-37: Reactive state approach is clear and extensible.
DefiningthemeStatewith Vue's reactive system is straightforward, and storing label-value pairs helps streamline usage.
65-72: Exporting composable with a default function.
Providing a custom composable return structure is a standard Vue 3 approach.packages/design-core/src/init.js (2)
24-25: Imports from meta-register are used appropriately.
These references togetMetaApi,META_SERVICElook correct for retrieving base info. No issues found.
42-42: Reusing localStorage with dynamic keys.
You’re dynamically building thetiny-engine-theme-${appId}key, which is good for multiple apps in one environment. This ensures no collisions in localStorage.Also applies to: 61-61
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/toolbars/themeSwitch/meta.js (1)
1-11: Consider adding type validation and internationalization.The configuration looks good but could benefit from:
- Type validation for the configuration object
- Internationalization for the title to support multiple languages
packages/toolbars/themeSwitch/src/composable/useThemeSwitch.js (1)
62-62: Simplify theme check.The optional chaining operator is unnecessary here as
themeState.themeis always defined.- const editorTheme = themeState.theme?.includes('dark') ? 'vs-dark' : 'vs' + const editorTheme = themeState.theme.includes('dark') ? 'vs-dark' : 'vs'packages/toolbars/themeSwitch/src/Main.vue (1)
33-36: Add validation for options prop.The options prop lacks validation for its structure. Consider adding proper validation to ensure required fields are present.
options: { type: Object, - default: () => ({}) + default: () => ({}), + validator: (value) => { + const requiredKeys = ['renderType'] // add required keys + return requiredKeys.every(key => key in value) + } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/design-core/assets/cn.svgis excluded by!**/*.svg
📒 Files selected for processing (17)
designer-demo/registry.js(3 hunks)jsconfig.json(2 hunks)packages/build/vite-config/src/vite-plugins/devAliasPlugin.js(1 hunks)packages/common/component/ToolbarBase.vue(3 hunks)packages/design-core/index.js(1 hunks)packages/design-core/package.json(1 hunks)packages/engine-cli/template/designer/registry.js(3 hunks)packages/layout/index.js(1 hunks)packages/register/src/constants.js(2 hunks)packages/toolbars/themeSwitch/index.js(1 hunks)packages/toolbars/themeSwitch/meta.js(1 hunks)packages/toolbars/themeSwitch/package.json(1 hunks)packages/toolbars/themeSwitch/src/Main.vue(1 hunks)packages/toolbars/themeSwitch/src/composable/index.js(1 hunks)packages/toolbars/themeSwitch/src/composable/useThemeSwitch.js(1 hunks)packages/toolbars/themeSwitch/src/styles/vars.less(1 hunks)packages/toolbars/themeSwitch/vite.config.js(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/toolbars/themeSwitch/src/styles/vars.less
- packages/toolbars/themeSwitch/vite.config.js
- packages/toolbars/themeSwitch/package.json
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/design-core/package.json
- packages/design-core/index.js
- packages/build/vite-config/src/vite-plugins/devAliasPlugin.js
- jsconfig.json
- packages/layout/index.js
- designer-demo/registry.js
- packages/common/component/ToolbarBase.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/toolbars/themeSwitch/src/composable/index.js (1)
4-11: LGTM! Well-structured service implementation.The service follows the MetaService pattern correctly and properly integrates with the meta-register system.
packages/toolbars/themeSwitch/index.js (1)
18-24: LGTM! Clean and modular exports.The module exports are well-structured, providing both bundled and individual access to the theme switch functionality.
packages/register/src/constants.js (1)
19-20: LGTM! Theme switch metadata properly integrated.The new ThemeSwitch entries are correctly added to both META_SERVICE and META_APP constants, following the established naming conventions and maintaining consistency with existing entries.
Also applies to: 30-30
packages/engine-cli/template/designer/registry.js (1)
24-24: LGTM! Theme switch properly integrated into registry.The ThemeSwitch component is correctly imported and integrated into both the toolbars array and layout configuration, maintaining consistency with the existing structure.
Also applies to: 68-68, 90-90
… feat/new-theme-switch
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/toolbars/themeSwitch/src/Main.vue (3)
3-3: Add TypeScript type definitions for toolbar-base props.The toolbar-base component props lack proper type definitions. Consider adding TypeScript interfaces or PropType definitions to improve type safety and developer experience.
+<script setup lang="ts"> +interface ToolbarBaseProps { + content: string + icon: string + options: Record<string, unknown> +} +</script> <template> <div class="toolbar-theme-switch"> - <toolbar-base :content="baseContent" :icon="baseIcon" :options="optionsData" @click-api="themeChange"> + <toolbar-base + :content="baseContent" + :icon="baseIcon" + :options="optionsData" + @click-api="themeChange">
27-34: Enhance props validation.The props definition could be improved with more specific types and validation rules.
options: { type: Object, - default: () => ({}) + default: () => ({}), + validator: (value) => { + // Add validation for required option properties + return true + } }, position: { type: String, default: 'right', + validator: (value) => ['right', 'collapse'].includes(value) }
37-40: Improve state management with reactive wrapper.The theme state management could be more robust by using Vue's reactive wrapper.
- const THEME_DATA = useThemeSwitch().THEME_DATA - const state = useThemeSwitch().themeState + const { THEME_DATA, themeState: state } = useThemeSwitch() + const reactiveState = reactive(state)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/component/ToolbarBase.vue(3 hunks)packages/toolbars/themeSwitch/src/Main.vue(1 hunks)packages/toolbars/themeSwitch/src/composable/index.js(1 hunks)packages/toolbars/themeSwitch/src/composable/useThemeSwitch.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/toolbars/themeSwitch/src/composable/index.js
- packages/common/component/ToolbarBase.vue
- packages/toolbars/themeSwitch/src/composable/useThemeSwitch.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/toolbars/themeSwitch/src/Main.vue (1)
57-58: Add theme validation before state change.The theme change logic should validate the theme value before applying the change to prevent potential runtime errors.
- const theme = useThemeSwitch().getTheme(state.theme).oppositeTheme - useThemeSwitch().themeChange(theme) + const currentTheme = useThemeSwitch().getTheme(state.theme) + if (!currentTheme) { + console.error(`Invalid theme: ${state.theme}`) + return + } + useThemeSwitch().themeChange(currentTheme.oppositeTheme)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/toolbars/themeSwitch/src/Main.vue (1)
38-39: Consider adding type safety for theme state initialization.The theme state initialization could benefit from type safety to prevent potential runtime errors.
- const { themeState: state, THEME_DATA, themeChange, getTheme, initThemeState } = useThemeSwitch() - initThemeState() + const { themeState: state, THEME_DATA, themeChange, getTheme, initThemeState } = useThemeSwitch() + // Add type safety + type ThemeState = ReturnType<typeof useThemeSwitch>['themeState'] + initThemeState() + + // Validate theme state after initialization + if (!state.theme || !THEME_DATA.some(theme => theme.value === state.theme)) { + console.warn('Invalid theme state detected') + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/themeSwitch/src/Main.vue(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/toolbars/themeSwitch/src/Main.vue (4)
6-6: Internationalize the hardcoded string "主题".The title text is hardcoded in Chinese. Consider using i18n for better internationalization support.
- <div class="toolbar-theme-switch-radio-title">主题</div> + <div class="toolbar-theme-switch-radio-title">{{ t('theme') }}</div>
7-8: Add ARIA labels for better accessibility.The radio group lacks proper accessibility attributes. Add ARIA labels to improve screen reader support.
- <tiny-radio-group v-model="state.theme" :options="THEME_DATA" class="theme-radio-group" @change="themeChange"> + <tiny-radio-group + v-model="state.theme" + :options="THEME_DATA" + class="theme-radio-group" + @change="themeChange" + aria-label="Theme Selection" + role="radiogroup"> </tiny-radio-group>
15-35: LGTM! Well-structured component definition.The component is well-organized with clear imports and properly typed props.
51-58: Add error handling for theme changes.The theme change functions lack error handling. Consider adding try-catch blocks and user feedback for failed theme changes.
const toChangeTheme = () => { if (props.position === COLLAPSE) { return } - const theme = getTheme(state.theme).oppositeTheme - themeChange(theme) + try { + const theme = getTheme(state.theme).oppositeTheme + themeChange(theme) + } catch (error) { + console.error('Failed to change theme:', error) + // Add user feedback mechanism here + } }
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
packages/toolbars/themeSwitch/src/Main.vue (3)
6-6: 🛠️ Refactor suggestionInternationalize the hardcoded string "主题".
The title text is hardcoded in Chinese. Consider using i18n for better internationalization support.
- <div class="toolbar-theme-switch-radio-title">主题</div> + <div class="toolbar-theme-switch-radio-title">{{ t('theme') }}</div>
7-8: 🛠️ Refactor suggestionAdd ARIA labels for better accessibility.
The radio group lacks proper accessibility attributes. Add ARIA labels to improve screen reader support.
- <tiny-radio-group v-model="state.theme" :options="THEME_DATA" class="theme-radio-group" @change="themeChange"> + <tiny-radio-group + v-model="state.theme" + :options="THEME_DATA" + class="theme-radio-group" + @change="themeChange" + aria-label="Theme Selection" + role="radiogroup"> </tiny-radio-group>
52-59: 🛠️ Refactor suggestionAdd error handling for theme changes.
The theme change function lacks error handling. Consider adding try-catch and user feedback for failed theme changes.
const toChangeTheme = () => { if (props.position === COLLAPSE) { return } - const theme = getTheme(state.theme).oppositeTheme - themeChange(theme) + try { + const theme = getTheme(state.theme).oppositeTheme + themeChange(theme) + } catch (error) { + console.error('Failed to change theme:', error) + // Add user feedback mechanism here + } }
🧹 Nitpick comments (2)
packages/toolbars/themeSwitch/src/composable/index.js (1)
28-30: Add validation in getTheme function.The function should validate the input parameter and throw a descriptive error for invalid themes.
const getTheme = (theme) => { + if (typeof theme !== 'string') { + throw new Error('Theme must be a string') + } return THEME_DATA.find((item) => theme === item.label) || DEFAULT_THEME }packages/toolbars/themeSwitch/src/Main.vue (1)
31-34: Add validation for position prop.The position prop should validate against allowed values.
position: { type: String, - default: 'right' + default: 'right', + validator: (value) => ['right', 'collapse'].includes(value) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/toolbars/themeSwitch/index.js(1 hunks)packages/toolbars/themeSwitch/src/Main.vue(1 hunks)packages/toolbars/themeSwitch/src/composable/index.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/toolbars/themeSwitch/index.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/design-core/src/preview/src/preview/Preview.vue (1)
64-66: Consider adding a default theme value.While the theme attribute is correctly set from query parameters, consider providing a default theme value to ensure consistent behavior when the theme parameter is not present.
const queryParams = getSearchParams() -document.documentElement?.setAttribute?.('data-theme', queryParams.theme) +document.documentElement?.setAttribute?.('data-theme', queryParams.theme || 'light')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/design-core/src/preview/src/preview/Preview.vue(1 hunks)packages/toolbars/preview/src/Main.vue(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/toolbars/preview/src/Main.vue (1)
Learnt from: yy-wow
PR: opentiny/tiny-engine#850
File: packages/toolbars/preview/src/Main.vue:16-16
Timestamp: 2024-11-10T16:32:54.790Z
Learning: In `packages/toolbars/preview/src/Main.vue`, within the `preview` function, the `getMergeMeta` method is used at lines 64 and 65 to retrieve `engine.config` configurations.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/toolbars/preview/src/Main.vue (2)
16-16: LGTM!The import statement has been updated to include the necessary theme management utilities.
81-82: LGTM! Theme state is properly integrated.The theme state is correctly retrieved and passed to the preview parameters, maintaining consistency with the existing parameter handling pattern.
Also applies to: 85-86
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
工具栏添加了主题切换插件
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Refactor