feat: Modify the theme switching logic and add a new theme template to CLI.#1143
feat: Modify the theme switching logic and add a new theme template to CLI.#1143chilingling merged 17 commits intoopentiny:developfrom
Conversation
WalkthroughThis pull request updates theme definitions across multiple packages by expanding the metadata of themes in the design registry. The CLI now supports a new command for creating themes with an associated template, which includes necessary configuration files and assets. Additionally, the theme switch functionality in the toolbar has been restructured to incorporate a popover for dynamic theme selection. The changes also refactor theme management logic to dynamically populate theme data and update associated CSS variables. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
ERR_PNPM_OPTIONAL_DEPS_REQUIRE_PROD_DEPS Optional dependencies cannot be installed without production dependencies ✨ Finishing Touches
🪧 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: 3
🧹 Nitpick comments (4)
packages/toolbars/themeSwitch/src/composable/index.js (1)
15-15: Reactive state definition
themeStateis defined as an empty reactive object. Consider initializing default properties here for clarity, though this approach also works fine.packages/toolbars/themeSwitch/src/Main.vue (3)
3-9: Reusable popover width
Consider extracting the hardcodedwidth="130"into a named constant or prop for easier maintenance. Otherwise, this is a clean setup.
10-19: Dynamically rendered theme list
This is straightforward. Consider adding anaria-labelor role for improved accessibility.
21-34: Integration with collapse view
Nice approach to show a radio group only undercollapse. For better i18n, consider translating the “主题” text if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/design-core/registry.js(1 hunks)packages/engine-cli/src/commands/create.js(1 hunks)packages/engine-cli/src/index.js(4 hunks)packages/engine-cli/template/theme/README.md(1 hunks)packages/engine-cli/template/theme/index.js(1 hunks)packages/engine-cli/template/theme/meta.js(1 hunks)packages/engine-cli/template/theme/package.json(1 hunks)packages/engine-cli/template/theme/src/styles/vars.less(1 hunks)packages/engine-cli/template/theme/vite.config.js(1 hunks)packages/toolbars/themeSwitch/src/Main.vue(3 hunks)packages/toolbars/themeSwitch/src/composable/index.js(3 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 (6)
- packages/engine-cli/template/theme/src/styles/vars.less
- packages/engine-cli/template/theme/README.md
- packages/engine-cli/template/theme/vite.config.js
- packages/engine-cli/template/theme/index.js
- packages/engine-cli/template/theme/package.json
- packages/engine-cli/template/theme/meta.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (18)
packages/toolbars/themeSwitch/src/styles/vars.less (1)
4-6: LGTM! Well-structured CSS variable definition.The new
.theme-listclass and its variable follow the established naming conventions and properly reuse existing theme variables.packages/engine-cli/src/index.js (3)
37-42: LGTM! Well-structured command implementation.The new
create-themecommand follows the established pattern and includes bilingual descriptions.
61-65: LGTM! Consistent theme option implementation.The theme option in the selection prompt maintains consistency with other options and includes clear descriptions.
82-83: LGTM! Clean typeMapper update.The typeMapper update is clean and maintains the object's structure.
packages/design-core/registry.js (2)
73-76: Looks good!
The introduction oftext,label,icon, andoppositeThemeclarifies theme metadata. Good job on making the theme definitions more descriptive.
80-83: Consistent approach for dark theme
Mirrors the structure used for the light theme. This ensures uniformity in how themes are defined and referenced.packages/toolbars/themeSwitch/src/composable/index.js (4)
2-8: Imports are well-structured
All relevant methods from@opentiny/tiny-engine-meta-registerare imported cleanly.
11-11: Use of mutable variable for dynamic data
DeclaringTHEME_DATAwithletis reasonable if it needs reassigning at runtime.
13-13: Initializing a null default theme
SettingDEFAULT_THEMEtonullbefore loading ensures safe initialization.
27-27: Icon assignment is consistent
AssigningthemeIconhere aligns with the rest of the theme properties. This keeps the data flow consistent.packages/toolbars/themeSwitch/src/Main.vue (8)
41-41: Use ofref
Importingrefis correct for managingshowpopover.
43-44: Popover import
ImportingPopoverfrom@opentiny/vuealigns with the new usage.
49-50: Component registration
TinyRadioGroupandTinyPopoverare properly registered.
75-76: UsingthemeIcon
Substitutingstate.themewithstate.themeIconimproves clarity.
77-86: Filtering for standard themes
themeShowTyperelies on checking if only light/dark exist. Confirm that custom themes with label ‘light’ or ‘dark’ won't break the logic.
97-103: Conditional popover logic
If only two themes exist, switch directly; otherwise, open the popover. This is an efficient solution.
105-108: Graceful popover closing
themeItemChangeproperly closes the popover after switching. Consider a safety check if the provided theme doesn’t exist, though likely not an issue.
126-139: Styling defines readable hover states
The.theme-itemhover and.activestyles are consistent with typical UI patterns. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/toolbars/themeSwitch/src/Main.vue (4)
3-9: Consider adding aria-label for accessibility.The popover should have an aria-label to improve accessibility for screen readers.
<tiny-popover width="130" trigger="manual" v-model="showpopover" :visible-arrow="false" popper-class="theme-popover" + aria-label="Theme Selection" >
11-18: Add keyboard navigation support.The theme list should be keyboard navigable for better accessibility.
<div class="theme-list"> <div v-for="item in THEME_DATA" :key="item.label" :class="['theme-item', { active: state.theme === item.label }]" @click="themeItemChange(item.label)" + tabindex="0" + role="button" + @keyup.enter="themeItemChange(item.label)" > {{ item.text }} </div>
80-87: Simplify theme type detection logic.The theme type detection can be simplified using array length comparison.
const themeShowType = computed(() => { - let filterList = THEME_DATA.filter((item) => item.label === 'light' || item.label === 'dark') || [] - if (THEME_DATA.length === filterList.length) { - return true - } else { - return false - } + const filterList = THEME_DATA.filter((item) => item.label === 'light' || item.label === 'dark') + return THEME_DATA.length === filterList.length })
127-138: Add transition for hover effect.Consider adding a smooth transition for the hover effect to improve the user experience.
.theme-list { .theme-item { padding: 4px 16px; margin: 0 -16px; + transition: background-color 0.2s ease; &:hover { background-color: var(--te-toolbar-theme-popover-list-item-bg-color-hover); } } .active { background-color: var(--te-toolbar-theme-popover-list-item-bg-color-hover); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/themeSwitch/src/Main.vue(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (1)
packages/toolbars/themeSwitch/src/Main.vue (1)
89-92:Details
❓ Verification inconclusive
Add error handling for missing opposite theme.
The
oppositeThemeproperty might be undefined for custom themes.
🏁 Script executed:
#!/bin/bash # Check if all themes have oppositeTheme defined ast-grep --pattern $'const theme = { $$$ oppositeTheme: $_, $$$ }'Length of output: 73
Update Error Handling for Missing
oppositeThemeThe current implementation in
packages/toolbars/themeSwitch/src/Main.vue:const toChangeTheme = () => { const theme = getTheme(state.theme).oppositeTheme themeChange(theme) }assumes that
getTheme(state.theme)always returns an object with a definedoppositeTheme. However, for custom themes that might omit this property, this can lead to runtime errors. Although our initial AST search didn’t return any occurrences (likely due to the structure of theme definitions being different), it's important to proactively verify and handle such cases.Suggested Action:
- Introduce a check to confirm that
oppositeThemeis defined before callingthemeChange(). For example, you could return an error message or fallback to a default value if it’s missing.Please manually verify that theme definitions across the codebase (e.g., those returned by
getTheme) indeed have proper fallbacks or defaults in caseoppositeThemeis undefined.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/toolbars/themeSwitch/src/Main.vue (3)
3-9: Consider making the popover width responsive or using min-width insteadThe popover width is hardcoded to "130", which might not accommodate all theme names, especially if custom themes with longer names are added.
- width="130" + min-width="130" + max-width="200"
24-24: Consider internationalizing the hardcoded textThe text "主题" (theme) is hardcoded in the template, which may cause issues for internationalization.
- <div class="toolbar-theme-switch-radio-title">主题</div> + <div class="toolbar-theme-switch-radio-title">{{ t('theme') }}</div>You would need to integrate with your i18n solution and import the translation function.
80-87: Avoid hardcoding theme names and consider caching the computationThe filtering logic hardcodes "light" and "dark" theme names, which could make the code less maintainable if theme identifiers change.
const themeShowType = computed(() => { - let filterList = THEME_DATA.filter((item) => item.label === 'light' || item.label === 'dark') || [] + const defaultThemes = ['light', 'dark'] // Consider moving to constants + let filterList = THEME_DATA.filter((item) => defaultThemes.includes(item.label)) || [] if (THEME_DATA.length === filterList.length) { return true } else { return false } })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/toolbars/themeSwitch/src/Main.vue(3 hunks)packages/toolbars/themeSwitch/src/styles/vars.less(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/toolbars/themeSwitch/src/styles/vars.less
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (9)
packages/toolbars/themeSwitch/src/Main.vue (9)
10-19: Good implementation of the theme list with active state trackingThe theme list implementation correctly uses v-for to iterate through available themes and tracks the active state by comparing with the current theme.
28-28: Good use of dynamic binding for vertical layoutUsing the themeShowType to determine the layout direction is a smart approach that adapts the UI based on available themes.
42-45: Import updates correctly support new functionalityThe addition of
refandPopoverimports properly supports the new reactive state and component needs.
77-78: Good transition from theme to themeIcon for icon displayChanging from
state.themetostate.themeIconsuggests a better separation of concerns where the theme identifier and its visual representation are decoupled.
94-103: Good decision logic for theme switching based on available themesThe implementation smartly determines whether to directly toggle between themes or show a popover based on the available themes, improving the user experience when multiple themes are available.
105-108: Good implementation of theme item selection with popover managementThe
themeItemChangemethod correctly handles both the theme change and popover visibility state.
110-122: Comprehensive return statement with all necessary functions and stateThe updated return statement properly includes all the new properties and methods needed by the template.
126-139:Details
✅ Verification successful
Good use of CSS variables for theme-specific styling
The styles properly use CSS variables for theming hover and active states, which allows for consistent theming across the application.
Make sure these CSS variables are defined in your theme stylesheet:
--te-toolbar-theme-popover-list-item-bg-color-hover--te-toolbar-theme-popover-list-item-bg-color-active
🏁 Script executed:
#!/bin/bash # Check if the CSS variables are defined rg --type=less "te-toolbar-theme-popover-list-item-bg-color"Length of output: 335
Final Verification: CSS Variables Defined Correctly
The CSS variables for theming (
--te-toolbar-theme-popover-list-item-bg-color-hoverand--te-toolbar-theme-popover-list-item-bg-color-active) are correctly defined inpackages/toolbars/themeSwitch/src/styles/vars.less, ensuring consistent theming across the application.
- File:
packages/toolbars/themeSwitch/src/styles/vars.less
--te-toolbar-theme-popover-list-item-bg-color-hoveris set tovar(--te-common-bg-container)--te-toolbar-theme-popover-list-item-bg-color-activeis similarly definedNo further changes are required.
89-92:Details
❓ Verification inconclusive
Verify that oppositeTheme is always available
The code now relies on the
oppositeThemeproperty being present in the theme data. Ensure this property is always defined for all themes to prevent runtime errors.
🏁 Script executed:
#!/bin/bash # Check if all themes have an oppositeTheme property ast-grep --pattern 'oppositeTheme'Length of output: 34
Below is a suggested next step. The initial ast‑grep run did not report any instances of “oppositeTheme,” which makes it hard to conclude whether all theme objects include it. Please run additional searches to locate both the theme definitions and the
getThemefunction to confirm if anoppositeThemeis always set. For example, you can run:#!/bin/bash # Search for any mention of "oppositeTheme" in the codebase rg "oppositeTheme" -n . # Identify where getTheme is imported/defined to locate its associated theme objects rg "import.*getTheme" -n .Once you confirm that every theme configuration (or the return value of getTheme) consistently defines an
oppositeThemeproperty, you can safely assume that the runtime is protected against related errors.Action Required: Confirm theme consistency regarding
oppositeTheme.
Please manually verify (or run similar scripts) that all theme objects include the requiredoppositeThemeproperty to ensure no runtime issues occur.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/engine-cli/src/commands/generateConfig.js (1)
32-43: Internationalization and indentation improvements needed.The function structure is good, but it has a few issues:
- The Chinese text '自定义主题' is hardcoded, making internationalization difficult
- The icon is hardcoded as 'dark' without any validation
- The template string has excessive indentation that will affect the output file's formatting
Consider using this improved implementation:
// 根据参数生成 config 文件内容 export const generateThemeMeta = (themeName = 'custom') => { const metaContent = ` - export default { - id: 'engine.theme.${themeName}', - text: '自定义主题', - label: '${themeName}', - icon: 'dark' - } +export default { + id: 'engine.theme.${themeName}', + text: '自定义主题', + label: '${themeName}', + icon: 'dark' +} ` return metaContent }Additionally, consider extracting the text into a configuration for better internationalization support.
packages/engine-cli/src/index.js (2)
37-52: Improve prompt message clarity and add themeName validation.The implementation is good, but the prompt message for theme name is confusing as it says "please enter the project name" but asks for a theme name.
program .command('create-theme <name>') .description('create a new tiny-engine theme 创建一个新的 tiny-engine 主题') .action(async (name) => { const themeName = await input({ - message: 'please enter the project name. 请输入主题名称', + message: 'please enter the theme name. 请输入主题名称', validate: (inputName) => { if (!inputName) { - return 'project name can not be empty. 主题名称不允许为空。' + return 'theme name can not be empty. 主题名称不允许为空。' } + + if (!/^[a-z0-9-]+$/.test(inputName)) { + return 'theme name can only contain lowercase letters, numbers, and hyphens. 主题名称只能包含小写字母、数字和连字符。' + } return true } }) createTheme(name, themeName) })
96-108: Fix inconsistent validation and prompt messaging.The same issue as in the create-theme command - the prompt message is confusing.
let themeName = '' if (type === 'theme') { themeName = await input({ - message: 'please enter the project name. 请输入主题名称', + message: 'please enter the theme name. 请输入主题名称', validate: (inputName) => { if (!inputName) { - return 'project name can not be empty. 主题名称不允许为空。' + return 'theme name can not be empty. 主题名称不允许为空。' } + + if (!/^[a-z0-9-]+$/.test(inputName)) { + return 'theme name can only contain lowercase letters, numbers, and hyphens. 主题名称只能包含小写字母、数字和连字符。' + } return true } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/design-core/registry.js(1 hunks)packages/engine-cli/src/commands/create.js(2 hunks)packages/engine-cli/src/commands/generateConfig.js(1 hunks)packages/engine-cli/src/index.js(4 hunks)packages/engine-cli/template/theme/meta.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine-cli/template/theme/meta.js
- packages/design-core/registry.js
🔇 Additional comments (6)
packages/engine-cli/src/commands/create.js (2)
17-17: Import updated correctly to include the new generateThemeMeta function.The import statement has been properly updated to include the newly added
generateThemeMetafunction.
66-81: Add error handling and validation to createTheme function.The function should validate the theme name and check for existing directories, similar to
createPlatform.Apply this diff to add the necessary checks:
export function createTheme(name, themeName) { + if (fs.pathExistsSync(path.join(cwd(), name))) { + logger.log(chalk.red(`create failed, because the ${name} folder already exists. 创建失败，${name} 文件夹已存在。`)) + return + } + const sourcePath = path.join(__dirname, '../template/theme/') const destPath = path.join(cwd(), name) - fs.copySync(sourcePath, destPath) + try { + fs.copySync(sourcePath, destPath) + + const configContent = generateThemeMeta(themeName) + + fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) + const content = fs.readFileSync(path.resolve(destPath, 'src/common.less'), 'utf-8') + const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) + fs.writeFileSync(path.resolve(destPath, 'src/common.less'), outputContent, 'utf-8') + + logger.log( + chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) + ) + } catch (error) { + logger.error(chalk.red(`Failed to create theme: ${error.message}`)) + } - - const configContent = generateThemeMeta(themeName) - - fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) - const content = fs.readFileSync(path.resolve(destPath, 'src/common.less'), 'utf-8') - const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) - fs.writeFileSync(path.resolve(destPath, 'src/common.less'), outputContent, 'utf-8') - - logger.log( - chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) - ) }packages/engine-cli/src/index.js (4)
14-14: Import statement correctly updated to include createTheme.The import statement has been properly updated to include the new
createThemefunction.
71-75: Theme option correctly added to selection choices.The theme option has been properly added to the selection choices in the create command.
90-94: typeMapper updated correctly to include the theme type.The typeMapper object has been properly updated to map the 'theme' type to the createTheme function.
110-110: Correctly passes themeName parameter to theme creation function.The code appropriately passes both the projectName and themeName to the type-specific creation function.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/engine-cli/src/commands/create.js (2)
66-82:⚠️ Potential issueAdd error handling and validation to createTheme function.
The function should validate the theme name and check for existing directories, similar to
createPlatform.Apply this diff to add the necessary checks:
export function createTheme(name, themeName) { + if (fs.pathExistsSync(path.join(cwd(), name))) { + logger.log(chalk.red(`create failed, because the ${name} folder already exists. 创建失败，${name} 文件夹已存在。`)) + return + } + const sourcePath = path.join(__dirname, '../template/theme/') const destPath = path.join(cwd(), name) - fs.copySync(sourcePath, destPath) + try { + fs.copySync(sourcePath, destPath) + logger.log( + chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) + ) + } catch (error) { + logger.error(chalk.red(`Failed to create theme: ${error.message}`)) + } - - logger.log( - chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) - ) }
73-77: 🛠️ Refactor suggestionUpdate additional files with the theme name.
Currently, only
common.lessis updated with the theme name.You should also update
package.json,vite.config.js, and any other files that may contain references to the theme name.Consider creating a function that handles these replacements across all relevant files:
+ function updateFileContents(destPath, themeName) { + // Update common.less + const themeLessPath = path.resolve(destPath, 'src/common.less') + let content = fs.readFileSync(themeLessPath, 'utf-8') + content = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) + fs.writeFileSync(themeLessPath, content, 'utf-8') + + // Update package.json + const packagePath = path.resolve(destPath, 'package.json') + const packageJson = fs.readJsonSync(packagePath) + packageJson.name = `theme-${themeName}` + packageJson.repository.directory = `packages/theme/${themeName}` + fs.writeJsonSync(packagePath, packageJson, { spaces: 2 }) + + // Update any other files as needed + }Then call this function after copying files:
fs.copySync(sourcePath, destPath) +updateFileContents(destPath, themeName)packages/engine-cli/src/index.js (1)
91-93: 🛠️ Refactor suggestionConsolidate project name and theme name prompts.
Currently, for themes, the user is prompted twice - once for project name and once for theme name.
Consider merging these prompts to improve user experience:
- const projectName = await setName() + const projectName = await setName(type === 'theme' ? 'project' : '') const typeMapper = { platform: createPlatform, plugin: createPlugin, theme: createTheme } - const themeName = type === 'theme' ? await setName(type) : '' + const themeName = type === 'theme' ? await input({ + message: 'please enter the theme name. 请输入主题名称', + validate: (inputName) => !inputName ? 'theme name cannot be empty. 主题名称不允许为空。' : true + }) : '' typeMapper[type](projectName, themeName)This would make the flow more intuitive by clearly distinguishing between the project folder name and the theme name.
packages/toolbars/themeSwitch/src/composable/index.js (1)
11-15:⚠️ Potential issueUpdate variable initialization to handle potential empty registry properly
The new dynamic initialization of
THEME_DATAandDEFAULT_THEMEis good, but there's a potential issue if the registry is empty. WhengetMergeRegistry('themes')returns an empty array, accessingTHEME_DATA[0]on line 44 will result inundefined.Add a fallback for when the themes registry is empty:
let THEME_DATA = [] let DEFAULT_THEME = null const themeState = reactive({})
🧹 Nitpick comments (7)
packages/engine-cli/template/theme/package.json (1)
1-29: Review the package.json template structure.The template provides a good foundation for theme packages. A few suggestions:
- Consider adding a
descriptionfield to help documentation systems and package registries.- Consider adding
peerDependenciesfor any required runtime dependencies of themes.- You might want to add some basic theme-related dependencies that would be common across all theme implementations.
Also consider adding a postinstall script that guides users on next steps for using the theme after installation.
packages/engine-cli/src/index.js (1)
50-56: Consider adding theme type selection.The current implementation only allows creating a custom theme with a user-provided name.
Consider enhancing this to allow users to select a theme type (dark, light, custom) similar to how the platform command supports theme selection:
program .command('create-theme <name>') .description('create a new tiny-engine theme 创建一个新的 tiny-engine 主题') + .addOption(new Option('-t, --type <type>', 'theme type 主题类型', 'custom').choices(['light', 'dark', 'custom'])) .action(async (name) => { const themeName = await setName('theme') - createTheme(name, themeName) + createTheme(name, themeName, options.type) })packages/toolbars/themeSwitch/src/composable/index.js (2)
20-21: Update getTheme function to handle theme objects and stringsThe
getThemefunction now searches bytypeproperty instead oflabel, but it doesn't handle the case wherethemeis an object instead of a string.Consider handling both cases:
const getTheme = (theme) => { - return THEME_DATA.find((item) => theme === item.type) || DEFAULT_THEME + if (typeof theme === 'string') { + return THEME_DATA.find((item) => theme === item.type) || DEFAULT_THEME + } else if (theme && theme.type) { + return theme + } + return DEFAULT_THEME }
24-27: Consider adding error handling to themeChange functionThe
themeChangefunction depends ongetThemereturning a valid theme object. If the registry is empty or the theme is not found, this could lead to runtime errors.Add error handling:
const themeChange = (theme) => { - themeState.theme = getTheme(theme).type - themeState.themeLabel = getTheme(themeState.theme).text - themeState.themeIcon = getTheme(themeState.theme).icon + const themeObj = getTheme(theme) + if (!themeObj) return + + themeState.theme = themeObj.type + themeState.themeLabel = themeObj.text + themeState.themeIcon = themeObj.icon document.documentElement.setAttribute('data-theme', themeState.theme)packages/toolbars/themeSwitch/src/Main.vue (3)
80-83: Improve themeShowType logic readabilityThe
themeShowTypecomputed property determines if all themes are either 'light' or 'dark', but the logic could be clearer.Consider simplifying and making the logic more explicit:
const themeShowType = computed(() => { - let filterList = THEME_DATA.filter((item) => ['light', 'dark'].includes(item.type)) || [] - return THEME_DATA.length === filterList.length + // Check if we only have light and dark themes (no custom themes) + const hasOnlyDefaultThemes = THEME_DATA.every((item) => ['light', 'dark'].includes(item.type)) + return hasOnlyDefaultThemes && THEME_DATA.length > 0 })
101-104: Enhance themeItemChange with animationThe
themeItemChangefunction works correctly, but you could enhance user experience by adding a slight delay before closing the popover.Consider adding a small delay:
const themeItemChange = (theme) => { themeChange(theme) - showpopover.value = false + // Add a small delay for better UX + setTimeout(() => { + showpopover.value = false + }, 100) }
122-135: Consider adding transitions for smoother theme switchingThe CSS styling is good, but adding transitions would enhance the user experience when switching themes.
Add transitions for smoother theme switching:
.theme-list { .theme-item { padding: 4px 16px; margin: 0 -16px; + transition: background-color 0.2s ease; &:hover { background-color: var(--te-toolbar-theme-popover-list-item-bg-color-hover); } } .active { background-color: var(--te-toolbar-theme-popover-list-item-bg-color-active); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/design-core/registry.js(1 hunks)packages/engine-cli/src/commands/create.js(2 hunks)packages/engine-cli/src/commands/generateConfig.js(1 hunks)packages/engine-cli/src/index.js(3 hunks)packages/engine-cli/template/theme/meta.js(1 hunks)packages/engine-cli/template/theme/package.json(1 hunks)packages/toolbars/themeSwitch/src/Main.vue(3 hunks)packages/toolbars/themeSwitch/src/composable/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/engine-cli/src/commands/generateConfig.js
- packages/design-core/registry.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (6)
packages/engine-cli/template/theme/meta.js (1)
1-7: Confirm the metadata structure meets requirements.The file sets up the metadata for a custom theme with an ID, display text, type and icon. However, I noticed the display text is in Chinese. Depending on your internationalization approach, you may want to consider:
- Using a more language-neutral value
- Adding internationalization support
- Documenting this expected format for developers
It appears this is a template file that will be customized during theme creation. Consider whether additional metadata properties might be needed for theme functionality or discovery.
packages/engine-cli/src/index.js (1)
18-29: Good extraction of common functionality.Creating the
setNamefunction to handle input validation for both project and theme names is a good approach.This makes the code more maintainable and follows the DRY principle.
packages/toolbars/themeSwitch/src/composable/index.js (1)
43-44: Add fallback for empty themes registryThere's no handling for the case when
getMergeRegistry('themes')returns an empty array.Add a fallback when the registry is empty:
THEME_DATA = getMergeRegistry('themes') -DEFAULT_THEME = THEME_DATA[0] +if (THEME_DATA.length > 0) { + DEFAULT_THEME = THEME_DATA[0] +} else { + DEFAULT_THEME = { type: 'light', icon: 'light', text: '浅色模式' } +}packages/toolbars/themeSwitch/src/Main.vue (3)
3-37: Good use of TinyPopover for theme selectionThe implementation of a popover for theme selection is a good improvement, providing a better user experience for selecting from multiple themes.
90-99: Good conditional logic for theme switchingThe implementation of
changeThemeTypeproperly handles different scenarios based on available themes, showing the popover only when custom themes are present.
85-88: Add null check for oppositeTheme propertyThe
toChangeThemefunction assumes that every theme has anoppositeThemeproperty, which might not be true for custom themes.Add a null check to handle cases where
oppositeThemeis not defined:const toChangeTheme = () => { - const theme = getTheme(state.theme).oppositeTheme + const currentTheme = getTheme(state.theme) + const theme = currentTheme.oppositeTheme || (currentTheme.type === 'light' ? 'dark' : 'light') themeChange(theme) }
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/engine-cli/template/theme/vite.config.js (2)
2-3: Update copyright year to 2024.The copyright year in this new file is 2023, while other files in the project use 2024. Consider updating for consistency.
-/** - * Copyright (c) 2023 - present TinyEngine Authors. - * Copyright (c) 2023 - present Huawei Cloud Computing Technologies Co., Ltd. +/** + * Copyright (c) 2024 - present TinyEngine Authors. + * Copyright (c) 2024 - present Huawei Cloud Computing Technologies Co., Ltd.
26-28: Consider using Vite's CSS handling instead of manual banner import.While the current approach works, Vite has built-in CSS handling capabilities that might be more maintainable.
rollupOptions: { output: { - banner: 'import "./style.css"' + assetFileNames: 'style.[ext]' } }Then in your
index.js:import './style.css' // rest of your code
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/engine-cli/src/commands/create.js(3 hunks)packages/engine-cli/src/commands/generateConfig.js(1 hunks)packages/engine-cli/template/theme/vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/engine-cli/src/commands/generateConfig.js
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (3)
packages/engine-cli/src/commands/create.js (2)
66-84: Add error handling and validation to createTheme function.The function should validate the theme name and check for existing directories, similar to
createPlatform.Apply this diff to add the necessary checks:
export function createTheme(name, themeName) { + if (fs.pathExistsSync(path.join(cwd(), name))) { + logger.log(chalk.red(`create failed, because the ${name} folder already exists. 创建失败，${name} 文件夹已存在。`)) + return + } + const sourcePath = path.join(__dirname, '../template/theme/') const destPath = path.join(cwd(), name) - fs.copySync(sourcePath, destPath) + try { + fs.copySync(sourcePath, destPath) + + const configContent = generateThemeMeta(themeName) + + const themePath = path.resolve(destPath, 'src/common.less') + fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) + const content = fs.readFileSync(themePath, 'utf-8') + const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) + fs.writeFileSync(themePath, outputContent, 'utf-8') + const pkgContent = generatePackageJson(themeName, sourcePath) + fs.outputJSONSync(path.resolve(destPath, 'package.json'), pkgContent, { spaces: 2 }) + + logger.log( + chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) + ) + } catch (error) { + logger.error(chalk.red(`Failed to create theme: ${error.message}`)) + } - - const configContent = generateThemeMeta(themeName) - - const themePath = path.resolve(destPath, 'src/common.less') - fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) - const content = fs.readFileSync(themePath, 'utf-8') - const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) - fs.writeFileSync(themePath, outputContent, 'utf-8') - const pkgContent = generatePackageJson(themeName, sourcePath) - fs.outputJSONSync(path.resolve(destPath, 'package.json'), pkgContent, { spaces: 2 }) - - logger.log( - chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) - ) }
46-46:Details
❓ Verification inconclusive
Verify the parameter change in generatePackageJson call.
The
mergedOptionsparameter has been removed from thegeneratePackageJsoncall. Ensure this change is intentional and compatible with the implementation ofgeneratePackageJsonin thegenerateConfig.jsfile.
🏁 Script executed:
#!/bin/bash # Check the signature of generatePackageJson in generateConfig.js grep -A 5 "export function generatePackageJson" packages/engine-cli/src/commands/generateConfig.jsLength of output: 100
🏁 Script executed:
#!/bin/bash # Search for the definition of generatePackageJson in the engine-cli directory. echo "Searching for 'export function generatePackageJson' occurrences:" rg "export function generatePackageJson" -n packages/engine-cli/ echo "-----" echo "Searching for any 'function generatePackageJson' occurrences (non-exported):" rg "function generatePackageJson" -n packages/engine-cli/ echo "-----" echo "Searching for references to 'mergedOptions' across the repository:" rg "mergedOptions" -n .Length of output: 827
Verify the generatePackageJson parameter change
The call on line 46 in
packages/engine-cli/src/commands/create.jsnow only passes(name, templatePath)togeneratePackageJsoninstead of also includingmergedOptions. Our search for an exported or local definition ofgeneratePackageJsonin the expected location (generateConfig.js) returned no results. We did see thatmergedOptionsis still being constructed and passed togenerateConfig, which suggests an API update might have occurred. Please manually verify the implementation ofgeneratePackageJson(and its intended signature) to ensure that removingmergedOptionsis an intentional change and that the function behaves as expected with the updated parameters.packages/engine-cli/template/theme/vite.config.js (1)
16-31: Configuration looks good for theme building.The Vite configuration is well-structured for building a theme package. It correctly:
- Disables public directory copying
- Sets up a library build with ES format
- Configures rollup to include CSS import in the output
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/engine-cli/src/index.js (2)
18-27: Good refactoring with the message mapCreating a message map to organize validation messages is a good practice. This addresses the previous review comment about using a Map to manage different message types.
However, consider using constants for the map keys to prevent typos:
+const THEME_TYPE = 'theme'; +const PROJECT_TYPE = 'project'; const messageMap = { - theme: { + [THEME_TYPE]: { message: 'please enter the theme name. 请输入主题名称', validateMessage: 'theme name can not be empty. 主题名称不允许为空。' }, - project: { + [PROJECT_TYPE]: { message: 'please enter the project name. 请输入项目名称', validateMessage: 'project name can not be empty. 项目名称不允许为空。' } }
29-40: The function rename improves code semanticsRenaming to
getNamefollowing the previous review comment makes the code more intuitive.Consider improving the type parameter usage:
-const getName = async (type) => { +const getName = async (isTheme = false) => { return await input({ - message: type ? messageMap.theme.message : messageMap.project.message, + message: isTheme ? messageMap.theme.message : messageMap.project.message, validate: (inputName) => { if (!inputName) { - return type ? messageMap.theme.validateMessage : messageMap.project.validateMessage + return isTheme ? messageMap.theme.validateMessage : messageMap.project.validateMessage } return true } }) }This makes it clearer that the parameter is a boolean flag for selecting theme-related messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/engine-cli/README.md(1 hunks)packages/engine-cli/src/index.js(3 hunks)packages/toolbars/themeSwitch/src/Main.vue(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/toolbars/themeSwitch/src/Main.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (4)
packages/engine-cli/README.md (1)
35-39: Good documentation for the new feature!The added documentation for creating a tiny-engine theme is clear and follows the same format as other commands in the README. This will help users understand how to use the new command.
packages/engine-cli/src/index.js (3)
61-67: Well-implemented new command for theme creationThe new
create-themecommand follows the same structure as other CLI commands and properly includes descriptions in both English and Chinese. Good implementation!
86-90: Good addition of the theme optionAdding the theme option to the choice list ensures users can create themes through the interactive prompt, consistent with the platform and plugin options.
99-99: Mapping for the new theme type is correctly addedThe typeMapper has been updated to include the createTheme function for the theme type.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/engine-cli/README.zh-CN.md (1)
36-41: Documentation Enhancement for Theme CreationThe new "创建主题" section clearly demonstrates how to create a theme using the CLI, which aligns nicely with the existing instructions for creating platforms and plugins. However, consider the following improvements:
Version Pinning: To comply with internal documentation guidelines (see retrieved learning), it may be preferable to reference the stable version (
@2.0.0) rather than@latest. This ensures consistency with the latest stable release.Suggested Diff:
-npx @opentiny/tiny-engine-cli@latest create-theme my-tiny-engine-theme +npx @opentiny/tiny-engine-cli@2.0.0 create-theme my-tiny-engine-themePost-Creation Instructions: Adding a brief note after the command—such as instructing users to navigate into the newly created theme directory and run
pnpm i—could help guide them to integrate the theme seamlessly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/engine-cli/README.zh-CN.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
packages/engine-cli/README.zh-CN.md (1)
Learnt from: gene9831
PR: opentiny/tiny-engine#917
File: docs/开始/快速上手.md:31-31
Timestamp: 2024-12-14T05:53:28.501Z
Learning: The latest stable version of `@opentiny/tiny-engine-cli` is `2.0.0`, and documentation should reference this version instead of any release candidates.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/engine-cli/src/index.js (1)
102-104:⚠️ Potential issueUpdate the
getNamefunction call to match the expected parameter type.When calling
getName(type), the type parameter is 'theme', not a boolean, which can lead to unexpected behavior since the function uses it as a boolean.- const themeName = type === 'theme' ? await getName(type) : '' + const themeName = type === 'theme' ? await getName(true) : ''packages/engine-cli/src/commands/create.js (1)
66-84:⚠️ Potential issueAdd error handling and validation to the createTheme function.
The function should validate the theme name and check for existing directories, similar to what
createPlatformdoes. This will prevent overwriting existing directories and provide better user feedback.export function createTheme(name, themeName) { + if (fs.pathExistsSync(path.join(cwd(), name))) { + logger.log(chalk.red(`create failed, because the ${name} folder already exists. 创建失败，${name} 文件夹已存在。`)) + return + } + const sourcePath = path.join(__dirname, '../template/theme/') const destPath = path.join(cwd(), name) - fs.copySync(sourcePath, destPath) + try { + fs.copySync(sourcePath, destPath) - const configContent = generateThemeMeta(themeName) + const configContent = generateThemeMeta(themeName) - const themePath = path.resolve(destPath, 'src/common.less') - fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) - const content = fs.readFileSync(themePath, 'utf-8') - const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) - fs.writeFileSync(themePath, outputContent, 'utf-8') - const pkgContent = generatePackageJson(name, sourcePath) - fs.outputJSONSync(path.resolve(destPath, 'package.json'), pkgContent, { spaces: 2 }) + const themePath = path.resolve(destPath, 'src/common.less') + fs.outputFileSync(path.resolve(destPath, 'meta.js'), configContent) + const content = fs.readFileSync(themePath, 'utf-8') + const outputContent = content.replace(/data-theme='custom'/g, `data-theme='${themeName}'`) + fs.writeFileSync(themePath, outputContent, 'utf-8') + const pkgContent = generatePackageJson(name, sourcePath) + fs.outputJSONSync(path.resolve(destPath, 'package.json'), pkgContent, { spaces: 2 }) - logger.log( - chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) - ) + logger.log( + chalk.green(`create finish, run the follow command to start project: \ncd ${name} && npm install && npm run dev`) + ) + } catch (error) { + logger.error(chalk.red(`Failed to create theme: ${error.message}`)) + } }packages/toolbars/themeSwitch/src/composable/index.js (2)
42-42:⚠️ Potential issueFix potential 'undefined' access to DEFAULT_THEME.type
Line 42 tries to access
DEFAULT_THEME.typebeforeDEFAULT_THEMEis initialized on line 44. If the user has a theme in localStorage or engine.config, this could cause a runtime error.Move this line after
DEFAULT_THEMEis initialized or add a null check:- localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || DEFAULT_THEME.type + localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || (DEFAULT_THEME && DEFAULT_THEME.type)
43-44:⚠️ Potential issueAdd fallback for empty themes registry
If
getMergeRegistry('themes')returns an empty array, accessing[0]will fail. Consider adding a fallback to avoid runtime errors.THEME_DATA.value = getMergeRegistry('themes') -DEFAULT_THEME = THEME_DATA.value[0] +if (THEME_DATA.value.length > 0) { + DEFAULT_THEME = THEME_DATA.value[0] +} else { + DEFAULT_THEME = { type: 'light', icon: 'light', text: '浅色模式' } +}
🧹 Nitpick comments (3)
packages/engine-cli/src/index.js (1)
29-40: Clarify function parameter type usage.The
getNamefunction usestypeas a boolean in its implementation, but it's later called with a string value ('theme'). This can be confusing and makes the code less maintainable.Consider renaming the parameter or updating the implementation to clarify that it's used as a boolean flag:
-const getName = async (type) => { +const getName = async (isTheme) => { return await input({ - message: type ? messageMap.theme.message : messageMap.project.message, + message: isTheme ? messageMap.theme.message : messageMap.project.message, validate: (inputName) => { if (!inputName) { - return type ? messageMap.theme.validateMessage : messageMap.project.validateMessage + return isTheme ? messageMap.theme.validateMessage : messageMap.project.validateMessage } return true } }) }packages/toolbars/themeSwitch/src/Main.vue (2)
83-86: Simplify themeShowType computed propertyThe
|| []is unnecessary sincefilter()always returns an array (possibly empty).const themeShowType = computed(() => { - let filterList = THEME_DATA.filter((item) => ['light', 'dark'].includes(item.type)) || [] + let filterList = THEME_DATA.filter((item) => ['light', 'dark'].includes(item.type)) return THEME_DATA.length === filterList.length })
88-91: Add safety check for oppositeTheme propertyThe code assumes all themes have an
oppositeThemeproperty. Add a check to handle cases where this property might be missing.const toChangeTheme = () => { - const theme = getTheme(state.theme).oppositeTheme + const currentTheme = getTheme(state.theme) + const theme = currentTheme.oppositeTheme || (currentTheme.type === 'light' ? 'dark' : 'light') themeChange(theme) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/engine-cli/src/commands/create.js(3 hunks)packages/engine-cli/src/index.js(3 hunks)packages/engine-cli/template/theme/package.json(1 hunks)packages/toolbars/themeSwitch/src/Main.vue(3 hunks)packages/toolbars/themeSwitch/src/composable/index.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/engine-cli/template/theme/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (12)
packages/engine-cli/src/index.js (3)
18-28: Good implementation of a structured message map.The creation of a
messageMapobject is a clean approach to organize messages by type. This makes the code more maintainable and scalable for future message types.
61-68: Good implementation of the theme creation command.The
create-themecommand follows the same pattern as the existing commands and provides a clear description of its purpose.
85-90: Good addition of the theme option.The theme option follows the same pattern as the existing options and provides a clear description.
packages/engine-cli/src/commands/create.js (2)
75-77: Ensure consistent theme name updates across all theme template files.The current replacement logic only updates
common.lesswith the theme name, but there may be other files (likepackage.json,meta.js,vite.config.js, etc.) that could also need theme name replacements.#!/bin/bash # Check all files in the theme template for potential places to replace 'custom' with the theme name cd packages/engine-cli/template/theme grep -r "custom" --include="*.json" --include="*.js" --include="*.less" .
46-47: Simplified function call.The updated
generatePackageJsoncall removes the unnecessarymergedOptionsparameter, making the code cleaner.packages/toolbars/themeSwitch/src/composable/index.js (2)
11-15: Updated theme data management to use reactive stateThe changes to make
THEME_DATAreactive and initializeDEFAULT_THEMEas null are good improvements that support dynamic theme loading. This allows for better reactivity in the UI when themes change.
20-22: Theme property naming changed from 'label' to 'type'The function now uses
item.typeinstead ofitem.labelfor theme identification. This is more semantically appropriate and matches the new theme property structure.packages/toolbars/themeSwitch/src/Main.vue (5)
3-37: Enhanced theme switching with popover UIThe new popover-based theme selection is a good improvement, providing a better user experience when multiple themes are available while maintaining the simple toggle for basic light/dark themes.
76-78: Updated theme list mapping to use 'type' instead of 'label'The mapping now correctly uses the
typeproperty for theme identification while keeping thelabelproperty for UI display compatibility with the radio component.
93-102: Good implementation of theme switching logicThe new
changeThemeTypefunction correctly implements the requirement to either toggle between light and dark directly or show a popover for multiple themes.
127-139: Added styles for theme popoverThe styles for the theme list and items are well-structured and use CSS variables for better theming support.
66-66:Details
❓ Verification inconclusive
Ensure getThemeData().value is always valid
Make sure
getThemeData()always returns a ref with a valid value property, as this code directly accesses it without any null checks.
🏁 Script executed:
#!/bin/bash # Check how getThemeData is implemented and used elsewhere rg -p "getThemeData\\(" --type jsLength of output: 124
Action Required: Validate getThemeData's Return Contract
At line 66 in
packages/toolbars/themeSwitch/src/Main.vue, we directly accessgetThemeData().valuewithout checking for null or undefined. Please confirm that the implementation ofgetThemeData()always returns a Vue ref with a guaranteed, validvalueproperty. If there's any possibility for the returned ref to be null or have an undefinedvalue, consider adding appropriate checks or safeguards.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/toolbars/themeSwitch/src/Main.vue (2)
88-91: Add error handling for theme oppositeTheme propertyThe method assumes that
getTheme(state.theme)will always return a valid theme object with anoppositeThemeproperty. Consider adding a fallback in case this property is undefined.const toChangeTheme = () => { - const theme = getTheme(state.theme).oppositeTheme + const themeObj = getTheme(state.theme) + const theme = themeObj?.oppositeTheme || (state.theme === 'light' ? 'dark' : 'light') themeChange(theme) }
76-78: Consider type validation for theme data mappingThe computed property assumes THEME_DATA.value is always an array. Consider adding a fallback for edge cases.
const radioThemeList = computed(() => { - return THEME_DATA.value.map((item) => ({ ...item, label: item.type })) + return Array.isArray(THEME_DATA.value) + ? THEME_DATA.value.map((item) => ({ ...item, label: item.type })) + : [] })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/themeSwitch/src/Main.vue(2 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)
3-37: Well-implemented popover-based theme selection!The new theme switching interface with
tiny-popoverhandles multiple themes elegantly. The implementation allows both direct switching between light/dark themes and a selection list for custom themes, which aligns well with the PR objectives.
83-86: Logic for determining theme show type looks correctThe computed property determines whether only light and dark themes are available by filtering the theme data. This correctly implements the requirement to show a popover when custom themes are available.
104-107: LGTM - Theme item change handler is concise and effectiveThe
themeItemChangemethod efficiently handles theme selection from the popover by updating the theme and closing the popover.
126-139: Well-structured theme styling using CSS variablesThe CSS implementation for theme items is clean and follows best practices by using CSS variables for theming.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/engine-cli/src/index.js (2)
103-104:⚠️ Potential issueIncorrect parameter type passed to getName function.
As noted in a previous review comment, the
getNamefunction expects a boolean flag, not a string parameter.-const themeName = type === 'theme' ? await getName(type) : '' +const themeName = type === 'theme' ? await getName(true) : ''
62-69: 🛠️ Refactor suggestionCommand implementation looks good, but parameter usage is incorrect.
The create-theme command is well-structured but there's an inconsistency with the
getNameparameter.program .command('create-theme <name>') .description('create a new tiny-engine theme 创建一个新的 tiny-engine 主题') .action(async (name) => { - const themeName = await getName('theme') + const themeName = await getName(true) createTheme(name, themeName) })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/engine-cli/src/index.js(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (5)
packages/engine-cli/src/index.js (5)
18-28: Map-based message handling is a good improvement.This implementation of a message map aligns with the suggestion from a previous review comment and provides a clean way to manage different message types based on context.
86-91: Good addition of theme option to selection prompt.This enhancement provides users with the ability to create themes through the interactive CLI interface.
95-95: Using getName function without arguments is correct for project names.This correctly calls the getName function without parameters to get project name prompts.
100-100: Good addition of theme mapping.Correctly maps the 'theme' type to the createTheme function.
1-14:Details
❌ Incorrect review comment
Consider adding README documentation for the new theme command.
As mentioned in a previous review comment (line 14), documentation should be added to the README file for the new theme creation functionality.
🏁 Script executed:
#!/bin/bash # Check if documentation for the theme command exists in README files grep -r "create-theme" --include="*.md" ./packages/engine-cli/Length of output: 262
Documentation for the new theme command is already present in the README files.
The grep results confirm that both
README.mdandREADME.zh-CN.mdin thepackages/engine-cli/directory include instructions for thecreate-themecommand. No additional documentation is required at this time.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/toolbars/themeSwitch/src/composable/index.js (1)
36-46: 🛠️ Refactor suggestionInitialization sequence needs improvement
The current initialization flow has inconsistencies that could lead to errors or unexpected behavior.
Consider restructuring the initialization to ensure proper sequence and safety:
init: () => { const appId = getMetaApi(META_SERVICE.GlobalService).getBaseInfo().id THEME_DATA.value = getMergeRegistry('themes') || [] // Set DEFAULT_THEME with fallback if (THEME_DATA.value.length > 0) { DEFAULT_THEME = THEME_DATA.value[0] } else { DEFAULT_THEME = { type: 'light', icon: 'light', text: '浅色模式' } } // Get theme preference with safe fallbacks const savedTheme = localStorage.getItem(`tiny-engine-theme-${appId}`) const configTheme = getMergeMeta('engine.config').theme const defaultType = DEFAULT_THEME.type || 'light' const theme = savedTheme || configTheme || defaultType themeChange(theme) }
♻️ Duplicate comments (1)
packages/toolbars/themeSwitch/src/composable/index.js (1)
40-45:⚠️ Potential issuePotential null reference when accessing DEFAULT_THEME.type
There's a risk of runtime error if
THEME_DATA.valueis empty asDEFAULT_THEMEwould be undefined when trying to accessDEFAULT_THEME.typeon line 44.Add a fallback for empty theme registry:
THEME_DATA.value = getMergeRegistry('themes') -DEFAULT_THEME = THEME_DATA.value[0] +if (THEME_DATA.value.length > 0) { + DEFAULT_THEME = THEME_DATA.value[0] +} else { + DEFAULT_THEME = { type: 'light', icon: 'light', text: '浅色模式' } +} const theme = - localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || DEFAULT_THEME.type + localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || DEFAULT_THEME?.type || 'light'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/themeSwitch/src/composable/index.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
🔇 Additional comments (2)
packages/toolbars/themeSwitch/src/composable/index.js (2)
21-21: Improved theme selection logic using 'type' instead of 'label'The switch from using 'label' to 'type' for theme identification aligns with the PR objective of improving theme switching logic. This is a better approach for clarity and maintainability.
24-28: Consistent theme property naming in themeChange functionThe themeChange function now properly sets the theme's icon along with its type and label, ensuring a more complete theme state representation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
packages/toolbars/themeSwitch/src/composable/index.js (2)
45-46:⚠️ Potential issueAdd fallback for empty themes registry
If
getMergeRegistry('themes')returns an empty array, accessing[0]will fail, which would cause runtime errors when trying to useDEFAULT_THEME.Add a fallback to handle the case when no themes are available in the registry:
THEME_DATA.value = getMergeRegistry('themes') -DEFAULT_THEME = THEME_DATA.value[0] +if (THEME_DATA.value.length > 0) { + DEFAULT_THEME = THEME_DATA.value[0] +} else { + DEFAULT_THEME = { type: 'light', icon: 'light', text: '浅色模式' } +}
48-48:⚠️ Potential issueFix potential 'undefined' access to DEFAULT_THEME.type
This line attempts to access
DEFAULT_THEME.typebefore ensuring that DEFAULT_THEME is properly initialized and not null.Add a null check before accessing the property:
-localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || DEFAULT_THEME.type +localStorage.getItem(`tiny-engine-theme-${appId}`) || getMergeMeta('engine.config').theme || (DEFAULT_THEME && DEFAULT_THEME.type)
🧹 Nitpick comments (3)
packages/toolbars/themeSwitch/src/composable/index.js (3)
11-13: Improve reactive state management consistencyYou've converted the theme data from static constants to reactive variables, which is good for dynamic theme management. However, using a mixture of
reffor THEME_DATA but a regular variable for DEFAULT_THEME could lead to inconsistent reactivity behavior.Consider making DEFAULT_THEME reactive as well for consistency:
let THEME_DATA = ref([]) -let DEFAULT_THEME = null +let DEFAULT_THEME = ref(null)Then update references accordingly:
-DEFAULT_THEME = THEME_DATA.value[0] +DEFAULT_THEME.value = THEME_DATA.value[0]
15-19: Initialize themeState with meaningful defaultsThe current initialization with empty strings could lead to undefined behavior if those values are used before proper initialization.
Consider initializing with defaults that match your expected light theme:
const themeState = reactive({ - theme: '', - themeLabel: '', - themeIcon: '' + theme: 'light', + themeLabel: '浅色模式', + themeIcon: 'light' })
29-29: Consider renaming theme properties for clarityAs noted in a previous comment, using
labelfor the theme value causes confusion. While you've switched to usingtypefor the theme value, there appears to be inconsistency in naming.Consider updating the property names throughout the codebase for better clarity and consistency:
-themeState.theme = getTheme(theme).type +themeState.themeType = getTheme(theme).typeAnd update all references to
themeState.themeaccordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/toolbars/themeSwitch/src/composable/index.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: push-check
…o CLI. (opentiny#1143) 1.修改主题切换逻辑，默认值改成从注册表中的themes获取。 只有亮色和暗色两种默认主题的情况下，保持原来的逻辑，点击直接切换主题 用户添加了自定义主题，出现选择列表，在列表中切换主题 2.添加新主题模板 在终端中，用命令创建新主题，选择theme类型，添加主题模板，输入主题名称：demo-theme 在demo-theme下可以看到新创建的主题，cd demo-theme && pnpm i 将主题接入设计器
…o CLI. (opentiny#1143) 1.修改主题切换逻辑，默认值改成从注册表中的themes获取。 只有亮色和暗色两种默认主题的情况下，保持原来的逻辑，点击直接切换主题 用户添加了自定义主题，出现选择列表，在列表中切换主题 2.添加新主题模板 在终端中，用命令创建新主题，选择theme类型，添加主题模板，输入主题名称：demo-theme 在demo-theme下可以看到新创建的主题，cd demo-theme && pnpm i 将主题接入设计器
3 participants
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
1.修改主题切换逻辑，默认值改成从注册表中的themes获取。
只有亮色和暗色两种默认主题的情况下，保持原来的逻辑，点击直接切换主题
用户添加了自定义主题，出现选择列表，在列表中切换主题
2.添加新主题模板
在终端中，用命令创建新主题，选择theme类型，添加主题模板，输入主题名称：demo-theme
在demo-theme下可以看到新创建的主题，cd demo-theme && pnpm i
将主题接入设计器
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
Summary by CodeRabbit