fix(site): add smb design-config support #2599
Conversation
WalkthroughThis pull request introduces several changes across multiple files, primarily focusing on the addition of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
github-actions
bot
added
bug
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
examples/sites/src/tools/useTheme.js (1)
Line range hint
52-57: Initialize SMB theme configuration in designConfigMapThe
SMB_THEMEentry indesignConfigMapis initialized with an empty object, but should probably usedesignSMBConfig.Consider updating the initialization:
const designConfigMap = { [DEFAULT_THEME]: {}, [INFINITY_THEME]: {}, [AURORA_THEME]: {}, - [SMB_THEME]: {} + [SMB_THEME]: designSMBConfig }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
examples/sites/package.json(1 hunks)examples/sites/src/tools/useTheme.js(2 hunks)packages/vue-common/src/index.ts(1 hunks)packages/vue/src/config-provider/src/index.vue(1 hunks)
🔇 Additional comments (4)
examples/sites/package.json (1)
28-28: LGTM: SMB design dependency added correctly
The addition of @opentiny/vue-design-smb as a workspace dependency is consistent with other design packages in the project.
examples/sites/src/tools/useTheme.js (1)
4-4: LGTM: SMB design config import added
The import statement for designSMBConfig is correctly added.
packages/vue/src/config-provider/src/index.vue (1)
61-62: LGTM: Proper reactive handling of design prop
The changes correctly handle the design prop:
- Using
toRefsensures proper reactivity - Destructuring simplifies the code while maintaining reactivity
packages/vue-common/src/index.ts (1)
177-179: 🛠️ Refactor suggestion
Consider the implications of mutable global state and add type safety
While the changes to handle reactive design configurations are necessary, there are a few considerations:
- Making
globalDesignConfigmutable could lead to race conditions if multiple components modify it simultaneously. - The type definition should be updated to reflect that the value could be reactive.
Consider these improvements:
- let globalDesignConfig: DesignConfig = customDesignConfig.designConfig || hooks.inject(design.configKey, {})
+ import type { ComputedRef } from 'vue'
+ let globalDesignConfig: DesignConfig | ComputedRef<DesignConfig> = customDesignConfig.designConfig || hooks.inject(design.configKey, {})Also, consider adding a comment to document the reactive object handling:
- // globalDesignConfig 可能是响应式对象，比如 computed
+ // Handle both reactive (ComputedRef) and non-reactive configurations
+ // This is necessary for supporting dynamic design configurations (e.g., SMB design)Let's verify if there are any other places in the codebase that might be affected by this change:
| if (router.currentRoute.value.params.theme === 'smb-theme') { | ||
| return designSMBConfig | ||
| } | ||
|
|
||
| return {} |
There was a problem hiding this comment.
Improve theme configuration fallback logic
The current implementation has several potential issues:
- Direct URL parameter check ('smb-theme') doesn't align with the
SMB_THEMEconstant - Missing fallback to
designConfigMap[currentThemeKey.value] - Empty object fallback could cause unexpected behavior
Consider this improved implementation:
if (import.meta.env.VITE_TINY_THEME === 'saas') {
return designSaasConfig
}
- if (router.currentRoute.value.params.theme === 'smb-theme') {
+ if (router.currentRoute.value.params.theme === THEME_ROUTE_MAP[SMB_THEME]) {
return designSMBConfig
}
- return {}
+ return designConfigMap[currentThemeKey.value] || {}Committable suggestion skipped: line range outside the PR's diff.
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
@opentiny/vue-design-smb.Bug Fixes
Documentation
ConfigProvidercomponent for better clarity and performance.