Repository navigation
feat: loading and spinner component with directive - #5587
Conversation
* 添加loading和spinner组件，以及对应的vue指令
|
WalkthroughThis update introduces new Vue directives for handling loading and spinning states across multiple applications. The changes update bootstrap files to register the new directives and adjust import statements accordingly. Additionally, the UI kit components for spinner and loading have been modified to change their initial rendering behaviors. The common UI package now exports these directives and components, and a new example route and view are added to showcase the loading functionality. Directive definitions include lifecycle hooks for mounting, updating, and unmounting the custom loading/spinner components. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
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: 8
🔭 Outside diff range comments (1)
packages/@core/ui-kit/shadcn-ui/src/components/spinner/loading.vue (1)
37-59: 🛠️ Refactor suggestionAdd error handling for timer cleanup.
The watch effect should clean up the timer in its stop function to prevent memory leaks.
watch( () => props.spinning, (show) => { if (!show) { showSpinner.value = false; clearTimeout(timer.value); return; } timer.value = setTimeout(() => { showSpinner.value = true; if (showSpinner.value) { renderSpinner.value = true; } }, props.minLoadingTime); }, { immediate: true, + onStop() { + clearTimeout(timer.value); + }, }, );
🧹 Nitpick comments (7)
packages/effects/common-ui/src/components/loading/spinner.vue (1)
10-10: Consider making min-height configurable.The hardcoded
min-h-20class might not be suitable for all use cases. Consider making it a prop to allow customization.packages/effects/common-ui/src/components/loading/loading.vue (1)
11-11: Consider making min-height configurable.The hardcoded
min-h-20class might not be suitable for all use cases. Consider making it a prop to allow customization.packages/effects/common-ui/src/components/loading/directive.ts (2)
38-46: Improve type safety in getOptions function.The function should explicitly type its return value and validate the object properties.
Consider this improvement:
-function getOptions(binding: DirectiveBinding) { +interface LoadingOptions { + spinning: boolean; + [key: string]: unknown; +} + +function getOptions(binding: DirectiveBinding): LoadingOptions { if (binding.value === undefined) { return { spinning: true }; } else if (typeof binding.value === 'boolean') { return { spinning: binding.value }; } else { - return { ...binding.value }; + return { spinning: true, ...binding.value }; } }
10-74: Reduce code duplication between directives.The loading and spinning directives share identical logic but operate on different components and symbols.
Consider creating a factory function:
function createLoadingDirective( component: typeof VbenLoading | typeof VbenSpinner, instanceKey: symbol ): Directive { return { mounted(el, binding) { const instance = h(component, getOptions(binding)); render(instance, el); el.classList.add('spinner-parent--relative'); el[instanceKey] = instance; }, unmounted(el) { const instance = el[instanceKey]; el.classList.remove('spinner-parent--relative'); render(null, el); instance.el.remove(); el[instanceKey] = null; }, updated(el, binding) { const instance = el[instanceKey]; const options = getOptions(binding); if (options && instance?.component) { Object.keys(options).forEach((key) => { instance.component.props[key] = options[key]; }); instance.component.update(); } }, }; } export const loadingDirective = createLoadingDirective( VbenLoading, LOADING_INSTANCE_KEY ); export const spinningDirective = createLoadingDirective( VbenSpinner, SPINNER_INSTANCE_KEY );packages/@core/ui-kit/shadcn-ui/src/components/spinner/loading.vue (1)
32-34: Consider using computed property for state derivation.The
showSpinnerandrenderSpinnerrefs are tightly coupled. Consider using a computed property forrenderSpinnerto derive its state fromshowSpinner, which would make the state management more predictable and reduce the chance of state synchronization issues.-const showSpinner = ref(false); -const renderSpinner = ref(false); +const showSpinner = ref(false); +const renderSpinner = computed(() => showSpinner.value);packages/@core/ui-kit/shadcn-ui/src/components/spinner/spinner.vue (1)
27-29: Consider extracting shared loading logic.The spinner component shares significant logic with the loading component. Consider extracting the common functionality into a composable.
// useLoading.ts export function useLoading(props: { spinning: boolean, minLoadingTime: number }) { const showSpinner = ref(false); const renderSpinner = computed(() => showSpinner.value); const timer = ref<ReturnType<typeof setTimeout>>(); watch( () => props.spinning, (show) => { if (!show) { showSpinner.value = false; clearTimeout(timer.value); return; } timer.value = setTimeout(() => { showSpinner.value = true; }, props.minLoadingTime); }, { immediate: true, onStop() { clearTimeout(timer.value); }, }, ); return { showSpinner, renderSpinner, }; }playground/src/views/examples/loading/index.vue (1)
8-12: Consider using a single state manager.Multiple similar state variables could be consolidated into a single state manager for better maintainability.
-const spinning = refAutoReset(false, 3000); -const loading = refAutoReset(false, 3000); -const spinningV = refAutoReset(false, 3000); -const loadingV = refAutoReset(false, 3000); +const loadingStates = reactive({ + spinning: refAutoReset(false, 3000), + loading: refAutoReset(false, 3000), + spinningV: refAutoReset(false, 3000), + loadingV: refAutoReset(false, 3000), +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
apps/web-antd/src/bootstrap.ts(2 hunks)apps/web-ele/src/bootstrap.ts(2 hunks)apps/web-naive/src/bootstrap.ts(2 hunks)packages/@core/ui-kit/shadcn-ui/src/components/spinner/loading.vue(3 hunks)packages/@core/ui-kit/shadcn-ui/src/components/spinner/spinner.vue(2 hunks)packages/effects/common-ui/src/components/index.ts(1 hunks)packages/effects/common-ui/src/components/loading/directive.ts(1 hunks)packages/effects/common-ui/src/components/loading/index.ts(1 hunks)packages/effects/common-ui/src/components/loading/loading.vue(1 hunks)packages/effects/common-ui/src/components/loading/spinner.vue(1 hunks)playground/src/bootstrap.ts(2 hunks)playground/src/router/routes/modules/examples.ts(1 hunks)playground/src/views/examples/loading/index.vue(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/effects/common-ui/src/components/loading/index.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (ubuntu-latest)
🔇 Additional comments (5)
packages/effects/common-ui/src/components/index.ts (1)
8-8: LGTM!The export statement follows the established pattern and correctly exposes the loading components.
apps/web-naive/src/bootstrap.ts (1)
4-8: LGTM!The import and registration of directives follow Vue.js best practices and are well-documented.
Also applies to: 38-41
apps/web-antd/src/bootstrap.ts (1)
4-8: LGTM! Clean implementation of directive registration.The changes properly integrate the loading and spinning directives with clear comments and appropriate placement in the bootstrap sequence.
Also applies to: 38-41
playground/src/bootstrap.ts (1)
4-8: LGTM! Consistent implementation across applications.The changes maintain consistency with the web-antd implementation, ensuring uniform directive availability.
Also applies to: 39-42
apps/web-ele/src/bootstrap.ts (1)
4-4: LGTM! Well-documented directive choices.The implementation provides flexibility by:
- Using Element Plus's loading directive by default
- Offering Vben's loading directive as an alternative
- Cleanly integrating the spinning directive
Also applies to: 36-40
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
packages/effects/common-ui/src/components/loading/directive.ts (4)
21-27:⚠️ Potential issueFix potential memory leak in unmount hook.
The unmount hook should call
render(null, el)to properly cleanup the VNode before removing the element.Apply this fix:
unmounted(el) { const instance = el[LOADING_INSTANCE_KEY]; el.classList.remove(CLASS_NAME_RELATIVE); + render(null, el); instance.el.remove(); el[LOADING_INSTANCE_KEY] = null; },
29-38:⚠️ Potential issueAdd error handling in updated hook.
The updated hook should handle potential errors during props update.
Apply this fix:
updated(el, binding) { const instance = el[LOADING_INSTANCE_KEY]; const options = getOptions(binding); if (options && instance?.component) { + try { Object.keys(options).forEach((key) => { instance.component.props[key] = options[key]; }); instance.component.update(); + } catch (error) { + console.error('Failed to update loading component:', error); + } } },
59-65:⚠️ Potential issueFix potential memory leak in unmount hook.
The unmount hook should call
render(null, el)to properly cleanup the VNode before removing the element.Apply this fix:
unmounted(el) { const instance = el[SPINNER_INSTANCE_KEY]; el.classList.remove(CLASS_NAME_RELATIVE); + render(null, el); instance.el.remove(); el[SPINNER_INSTANCE_KEY] = null; },
67-76:⚠️ Potential issueAdd error handling in updated hook.
The updated hook should handle potential errors during props update.
Apply this fix:
updated(el, binding) { const instance = el[SPINNER_INSTANCE_KEY]; const options = getOptions(binding); if (options && instance?.component) { + try { Object.keys(options).forEach((key) => { instance.component.props[key] = options[key]; }); instance.component.update(); + } catch (error) { + console.error('Failed to update spinner component:', error); + } } },
🧹 Nitpick comments (3)
apps/web-ele/src/bootstrap.ts (1)
36-40: Consider documenting the potential directive conflict.The registration logic is correct, and the comment explains the configuration. However, it would be helpful to document the potential conflict between Element Plus's and Vben's v-loading directives in the codebase's documentation to help future maintainers understand why only one should be enabled.
packages/effects/common-ui/src/components/loading/directive.ts (2)
41-49: Add type validation for object values.Consider adding type validation to ensure object values contain valid props for the components.
Apply this enhancement:
function getOptions(binding: DirectiveBinding) { if (binding.value === undefined) { return { spinning: true }; } else if (typeof binding.value === 'boolean') { return { spinning: binding.value }; } else { + const value = binding.value as Record<string, unknown>; + // Add validation for known props + const validProps = ['spinning', 'size', 'text']; + Object.keys(value).forEach((key) => { + if (!validProps.includes(key)) { + console.warn(`Unknown prop '${key}' passed to loading/spinner component`); + } + }); return { ...binding.value }; } }
91-116: Improve style injection to handle duplicates.The style injection should check for existing styles to prevent duplicates during hot reloads or multiple registrations.
Apply this enhancement:
export function registerLoadingDirective( app: App, params?: loadingDirectiveParams, ) { - const style = document.createElement('style'); - style.id = CLASS_NAME_RELATIVE; - style.innerHTML = ` - .${CLASS_NAME_RELATIVE} { - position: relative !important; - } - `; - document.head.append(style); + // Check if style already exists + if (!document.getElementById(CLASS_NAME_RELATIVE)) { + const style = document.createElement('style'); + style.id = CLASS_NAME_RELATIVE; + style.innerHTML = ` + .${CLASS_NAME_RELATIVE} { + position: relative !important; + } + `; + document.head.append(style); + } if (params?.loading !== false) { app.directive( isString(params?.loading) ? params.loading : 'loading', loadingDirective, ); } if (params?.spinning !== false) { app.directive( isString(params?.spinning) ? params.spinning : 'spinning', spinningDirective, ); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web-antd/src/bootstrap.ts(2 hunks)apps/web-ele/src/bootstrap.ts(2 hunks)apps/web-naive/src/bootstrap.ts(2 hunks)packages/effects/common-ui/src/components/loading/directive.ts(1 hunks)packages/effects/common-ui/src/components/loading/loading.vue(1 hunks)packages/effects/common-ui/src/components/loading/spinner.vue(1 hunks)playground/src/bootstrap.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- playground/src/bootstrap.ts
- apps/web-antd/src/bootstrap.ts
- packages/effects/common-ui/src/components/loading/loading.vue
- packages/effects/common-ui/src/components/loading/spinner.vue
- apps/web-naive/src/bootstrap.ts
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
apps/web-ele/src/bootstrap.ts (1)
4-4: LGTM!The import statement is correctly placed and follows naming conventions.
packages/effects/common-ui/src/components/loading/directive.ts (1)
1-12: LGTM!The imports are well-organized, and the use of symbols for instance keys prevents naming conflicts. The CSS class name is properly defined as a constant for reusability.
Description
添加loading和spinner组件，以及对应的vue指令
close #5507
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
New Features
Refactor