feat: Modify the theme switching logic and add a new theme template to CLI. #1143
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
Sequence Diagram(s)sequenceDiagram
participant User as "User"
participant CLI as "CLI"
participant FS as "File System"
User->>CLI: Run "create-theme <name>" command
CLI->>CLI: Invoke createTheme(name)
CLI->>FS: Copy theme template files
FS-->>CLI: Confirm copy
CLI->>User: Log completion message
sequenceDiagram
participant User
participant ThemeSwitch as "Theme Switch Component"
participant State as "Theme State"
User->>ThemeSwitch: Click toolbar button
ThemeSwitch->>ThemeSwitch: Execute changeThemeType()
alt Show Popover
ThemeSwitch->>User: Display tiny-popover with theme list
User->>ThemeSwitch: Select a theme
ThemeSwitch->>State: Invoke themeItemChange(theme)
State-->>ThemeSwitch: Update theme and themeIcon
ThemeSwitch->>User: Close popover
else Direct Change
ThemeSwitch->>State: Apply theme change directly
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 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:❓ 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:✅ 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:❓ 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.
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