feat(pager): reconstruct the multi-terminal template of the pager component from the vue template #3422
Conversation
WalkthroughThis update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileFirstPager
participant ParentComponent
User->>MobileFirstPager: Interact with pager controls (prev/next, size, jumper)
MobileFirstPager->>MobileFirstPager: Update reactive state, validate input
MobileFirstPager->>ParentComponent: Emit events (before-change, page-change, size-change, etc.)
ParentComponent-->>MobileFirstPager: Update props (currentPage, pageSize, total)
MobileFirstPager->>MobileFirstPager: Sync internal state with props via watchers
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
packages/vue/src/pager/src/mobile-first.vueOops! Something went wrong! :( ESLint: 8.57.1 ESLint couldn't find the plugin "eslint-plugin-vue". (The package "eslint-plugin-vue" was not found when loaded as a Node module from the directory "".) It's likely that the plugin isn't installed correctly. Try reinstalling by running the following: The plugin "eslint-plugin-vue" was referenced from the config file in ".eslintrc.js » @antfu/eslint-config » @antfu/eslint-config-vue". If you still can't figure out the problem, please stop by https://eslint.org/chat/help to chat with the team. Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 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 (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| <input | ||
| ref="jumperInput" | ||
| type="tel" | ||
| :disabled="disabled" |
There was a problem hiding this comment.
Ensure that the input value is validated correctly to prevent any potential security vulnerabilities such as injection attacks. Consider using a more robust validation method.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/vue/package.json (1)
240-240: Preserve POSIX-style trailing newlineRemoving the final newline is harmless for Node, but most *nix tools (git-diff, POSIX text utilities, editors) expect every text file to end with
\n. Keeping it avoids spurious diffs like this one.packages/vue/src/pager/src/mobile-first.vue (2)
680-687:internalPageCountcoerces to ≥ 1 – edge-case for empty datasetsWhen
total === 0,internalPageCountbecomes1, so the pager shows a single page even though there are no records. This can confuse users and defeatshideOnSinglePage(it never hides).Consider returning
0when there is no page at all and update the UI guards accordingly.
272-799: Component is >750 LOC – extract composables for maintainabilityBusiness logic (jumper, size selector, navigation) is intertwined with rendering, making the file hard to reason about and test. Splitting the reactive state & handlers into
useJumper(),usePageSize(), etc., would:• improve cohesion
• enable unit-testing of logic without mounting the component
• allow reuse in desktop/other variants of the pager.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/vue/package.json(1 hunks)packages/vue/src/pager/src/mobile-first.vue(8 hunks)packages/vue/src/popover/src/mobile-first.vue(0 hunks)
💤 Files with no reviewable changes (1)
- packages/vue/src/popover/src/mobile-first.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (1)
packages/vue/src/pager/src/mobile-first.vue (1)
712-739: Watch-loop risk: emittingupdate:currentPageimmediately after syncing the prop
watch(props.currentPage)setsinternalCurrentPage, and the next watcher immediately emitsupdate:currentPage, re-notifying the parent with the same value.
Although Vue’s diffing prevents an infinite loop, it still produces an unnecessary extra event per change.Consider either
• removing the second watcher’simmediate: true, or
• gating the emit with a value difference check.
| emit('update:pageSize', val) | ||
| emit('size-change', val) | ||
| emit('page-change', { | ||
| currentPage: state.internalCurrentPage, | ||
| pageSize: val, | ||
| total: state.internalTotal | ||
| }) | ||
| if (sizesList.value) { | ||
| sizesList.value.hide() | ||
| } | ||
| } | ||
| }, | ||
| render() { | ||
| return h('div', { class: ['h-7 leading-7 inline-block align-middle text-xs'] }, [ | ||
| h('div', { class: ['text-[0] h-7'] }, [ | ||
| h( | ||
| 'span', | ||
| { | ||
| class: ['text-xs pl-4 pr-2 text-color-text-primary'] | ||
| }, | ||
| [t('ui.page.jump')] | ||
| ), | ||
| h('input', { | ||
| class: [ | ||
| 'w-8 h-7 text-center align-top rounded-sm inline-block border border-solid border-color-border hover:text-color-icon-primary hover:border-color-icon-primary', | ||
| 'text-color-text-primary text-xs transition-[border] duration-300 outline-0 box-border mr-0 focus:border-color-border-focus' | ||
| ], | ||
| domProps: { | ||
| value: this.value | ||
| }, | ||
| attrs: { | ||
| type: 'tel', | ||
| disabled: this.disabled | ||
| }, | ||
| on: { | ||
| focus: this.handleFocus, | ||
| input: this.handleInput, | ||
| change: this.handleChange | ||
| }, | ||
| ref: 'input' | ||
| }) | ||
| ]) | ||
| ]) | ||
| } | ||
| }, | ||
| Total: { | ||
| mounted() { | ||
| if (document.querySelector('[data-tag="tiny-pager-total-loading"]')) { | ||
| Loading.service({ | ||
| target: document.querySelector('[data-tag="tiny-pager-total-loading"]') | ||
| }) | ||
|
|
||
| if (props.isBeforePageChange) { | ||
| const newPageSize = val | ||
| const currentPageSize = state.internalPageSize | ||
| const params = { newPageSize, currentPageSize, callback } | ||
| beforeSizeChangeHandler(params) | ||
| } else { | ||
| callback() | ||
| } | ||
| }, | ||
| render() { | ||
| return typeof this.$parent.internalTotal === 'number' ? ( | ||
| this.$parent.showTotalLoading ? ( | ||
| <div class="inline-block align-middle text-xs h-7 leading-7 float-left"> | ||
| <div class="h-7 leading-7 text-xs text-color-text-primary"> | ||
| <div | ||
| data-tag="tiny-pager-total-loading" | ||
| class="inline-block align-baseline h-3.5 w-3.5 mr-1.5 top-0.5 [&_[data-tag=tiny-loading-icon]]:h-3.5 [&_[data-tag=tiny-loading-icon]]:w-3.5"></div> | ||
| <span class="text-color-text-secondary">{t('ui.page.loadingTotals')}</span> | ||
| </div> | ||
| </div> | ||
| ) : ( | ||
| <div class="inline-block align-middle text-xs h-7 leading-7 float-left"> | ||
| {' '} | ||
| <div class="h-7 leading-7 text-xs text-color-text-primary"> | ||
| <span>{t('ui.page.total')}</span> | ||
| <span class="my-0 mx-1"> | ||
| {this.$parent.customTotal ? this.$parent.totalText : this.$parent.internalTotal} | ||
| </span> | ||
| <span>{t('ui.page.item')}</span> | ||
| </div> | ||
| </div> | ||
| ) | ||
| ) : ( | ||
| '' | ||
| ) | ||
| } | ||
| }, | ||
| Pager | ||
| }, | ||
| methods: { | ||
| canJumperGo() { | ||
| const inputValue = Number(this.$refs.jumper.$refs.input.value || 0) | ||
| const currentPage = Number(this.internalCurrentPage || 0) | ||
| return this.accurateJumper ? inputValue !== currentPage : true | ||
| }, | ||
| beforeSizeChangeHandler(params) { | ||
| const { newPageSize, currentPageSize, callback } = params | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
beforeChangeHandler() receives a dummy value, breaking consumers that rely on the new page number
Inside handleSizeChange the pre-change guard is invoked with its default -1, not the actual upcoming page (state.internalCurrentPage), so any listener comparing newPage with the current page will mis-behave.
- if (!beforeChangeHandler()) {
+ if (!beforeChangeHandler(state.internalCurrentPage)) {Pass the real page so downstream hooks get accurate context.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSizeChange = (val: number) => { | |
| if (val !== state.internalPageSize) { | |
| const callback = () => { | |
| if (!beforeChangeHandler()) { | |
| return false | |
| } | |
| if (value >= max) { | |
| this.value = max | |
| } else if (value <= min) { | |
| this.value = min | |
| } else { | |
| this.value = value | |
| state.internalPageSize = val | |
| state.userChangePageSize = true | |
| state.showSizes = false | |
| emit('update:pageSize', val) | |
| emit('size-change', val) | |
| emit('page-change', { | |
| currentPage: state.internalCurrentPage, | |
| pageSize: val, | |
| total: state.internalTotal | |
| }) | |
| if (sizesList.value) { | |
| sizesList.value.hide() | |
| } | |
| } | |
| }, | |
| render() { | |
| return h('div', { class: ['h-7 leading-7 inline-block align-middle text-xs'] }, [ | |
| h('div', { class: ['text-[0] h-7'] }, [ | |
| h( | |
| 'span', | |
| { | |
| class: ['text-xs pl-4 pr-2 text-color-text-primary'] | |
| }, | |
| [t('ui.page.jump')] | |
| ), | |
| h('input', { | |
| class: [ | |
| 'w-8 h-7 text-center align-top rounded-sm inline-block border border-solid border-color-border hover:text-color-icon-primary hover:border-color-icon-primary', | |
| 'text-color-text-primary text-xs transition-[border] duration-300 outline-0 box-border mr-0 focus:border-color-border-focus' | |
| ], | |
| domProps: { | |
| value: this.value | |
| }, | |
| attrs: { | |
| type: 'tel', | |
| disabled: this.disabled | |
| }, | |
| on: { | |
| focus: this.handleFocus, | |
| input: this.handleInput, | |
| change: this.handleChange | |
| }, | |
| ref: 'input' | |
| }) | |
| ]) | |
| ]) | |
| } | |
| }, | |
| Total: { | |
| mounted() { | |
| if (document.querySelector('[data-tag="tiny-pager-total-loading"]')) { | |
| Loading.service({ | |
| target: document.querySelector('[data-tag="tiny-pager-total-loading"]') | |
| }) | |
| if (props.isBeforePageChange) { | |
| const newPageSize = val | |
| const currentPageSize = state.internalPageSize | |
| const params = { newPageSize, currentPageSize, callback } | |
| beforeSizeChangeHandler(params) | |
| } else { | |
| callback() | |
| } | |
| }, | |
| render() { | |
| return typeof this.$parent.internalTotal === 'number' ? ( | |
| this.$parent.showTotalLoading ? ( | |
| <div class="inline-block align-middle text-xs h-7 leading-7 float-left"> | |
| <div class="h-7 leading-7 text-xs text-color-text-primary"> | |
| <div | |
| data-tag="tiny-pager-total-loading" | |
| class="inline-block align-baseline h-3.5 w-3.5 mr-1.5 top-0.5 [&_[data-tag=tiny-loading-icon]]:h-3.5 [&_[data-tag=tiny-loading-icon]]:w-3.5"></div> | |
| <span class="text-color-text-secondary">{t('ui.page.loadingTotals')}</span> | |
| </div> | |
| </div> | |
| ) : ( | |
| <div class="inline-block align-middle text-xs h-7 leading-7 float-left"> | |
| {' '} | |
| <div class="h-7 leading-7 text-xs text-color-text-primary"> | |
| <span>{t('ui.page.total')}</span> | |
| <span class="my-0 mx-1"> | |
| {this.$parent.customTotal ? this.$parent.totalText : this.$parent.internalTotal} | |
| </span> | |
| <span>{t('ui.page.item')}</span> | |
| </div> | |
| </div> | |
| ) | |
| ) : ( | |
| '' | |
| ) | |
| } | |
| }, | |
| Pager | |
| }, | |
| methods: { | |
| canJumperGo() { | |
| const inputValue = Number(this.$refs.jumper.$refs.input.value || 0) | |
| const currentPage = Number(this.internalCurrentPage || 0) | |
| return this.accurateJumper ? inputValue !== currentPage : true | |
| }, | |
| beforeSizeChangeHandler(params) { | |
| const { newPageSize, currentPageSize, callback } = params | |
| } | |
| const handleSizeChange = (val: number) => { | |
| if (val !== state.internalPageSize) { | |
| const callback = () => { | |
| if (!beforeChangeHandler(state.internalCurrentPage)) { | |
| return false | |
| } | |
| state.internalPageSize = val | |
| state.userChangePageSize = true | |
| state.showSizes = false | |
| emit('update:pageSize', val) | |
| emit('size-change', val) | |
| emit('page-change', { | |
| currentPage: state.internalCurrentPage, | |
| pageSize: val, | |
| total: state.internalTotal | |
| }) | |
| if (sizesList.value) { | |
| sizesList.value.hide() | |
| } | |
| } | |
| if (props.isBeforePageChange) { | |
| const newPageSize = val | |
| const currentPageSize = state.internalPageSize | |
| const params = { newPageSize, currentPageSize, callback } | |
| beforeSizeChangeHandler(params) | |
| } else { | |
| callback() | |
| } | |
| } | |
| } |
| // 生命周期钩子 | ||
| onMounted(() => { | ||
| if (document.querySelector('[data-tag="tiny-pager-total-loading"]')) { | ||
| const target = document.querySelector('[data-tag="tiny-pager-total-loading"]') | ||
| if (target) { | ||
| Loading.service({ | ||
| target: target as HTMLElement | ||
| }) | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
Loading.service instance is never disposed – potential memory leak & redundant DOM query
onMounted reaches into the DOM twice with document.querySelector, creates a Loading.service, and never tears it down.
• Re-querying the DOM is brittle – the target can be obtained via a ref.
• Without calling close() on the service in onUnmounted, the overlay and its timers stay in memory when the component is destroyed.
- onMounted(() => {
- if (document.querySelector('[data-tag="tiny-pager-total-loading"]')) {
- const target = document.querySelector('[data-tag="tiny-pager-total-loading"]')
- if (target) {
- Loading.service({
- target: target as HTMLElement
- })
- }
- }
- })
+ const loadingInstance = ref<any>(null)
+
+ onMounted(() => {
+ const target = document.querySelector('[data-tag="tiny-pager-total-loading"]') as HTMLElement | null
+ if (target) {
+ loadingInstance.value = Loading.service({ target })
+ }
+ })
+
+ onUnmounted(() => {
+ loadingInstance.value?.close?.()
+ })Also extend the hooks destructuring to include onUnmounted.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 生命周期钩子 | |
| onMounted(() => { | |
| if (document.querySelector('[data-tag="tiny-pager-total-loading"]')) { | |
| const target = document.querySelector('[data-tag="tiny-pager-total-loading"]') | |
| if (target) { | |
| Loading.service({ | |
| target: target as HTMLElement | |
| }) | |
| } | |
| } | |
| }) | |
| // 生命周期钩子 | |
| const loadingInstance = ref<any>(null) | |
| onMounted(() => { | |
| const target = document.querySelector('[data-tag="tiny-pager-total-loading"]') as HTMLElement | null | |
| if (target) { | |
| loadingInstance.value = Loading.service({ target }) | |
| } | |
| }) | |
| onUnmounted(() => { | |
| loadingInstance.value?.close?.() | |
| }) |
zzcr
changed the title
feat(pager): 将pager组件多端模板重构从vue模板，并改成成setup函数格式
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
Refactor
Accessibility
Style
Chores