feat(pager): [pager] add change-compat to control whether trigger cha… #2783
Conversation
…nge event when value change
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
participant User
participant Pager
participant EventHandler
User->>Pager: Change page/size
alt changeCompat is false
Pager->>EventHandler: No automatic event
else changeCompat is true
Pager->>EventHandler: Trigger change events
EventHandler-->>User: Notify of changes
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Finishing Touches
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 (
|
Walkthrough此PR为分页组件添加了一个新的特性 Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| (newVal: number[]): void => { | ||
| if (Array.isArray(newVal)) { | ||
| state.internalPageSize = newVal.includes(props.pageSize) ? props.pageSize : newVal[0] | ||
| if (props.changeCompat) { |
There was a problem hiding this comment.
The condition if (props.changeCompat) is used to control whether the update:current-page and current-change events are emitted. This logic ensures that these events are only triggered when changeCompat is true, aligning with the new feature's purpose.
WalkthroughThis PR adds a new feature Changes
|
![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 (5)
packages/vue/src/pager/src/index.ts (1)
71-76: Consider improving the documentation of the new property.The comment "tiny新增" should be in English for consistency. Consider adding JSDoc documentation to describe the purpose and behavior of this property.
- // tiny新增 + /** + * Controls whether to trigger change events when value changes manually. + * @default false + */ changeCompat: { type: Boolean, default: false }examples/sites/demos/pc/app/pager/pager-event-composition-api.vue (1)
76-81: Consider enhancing the event feedback.The message could be more informative by including the new page value.
const dataChangeTrigger = () => { TinyModal.message({ - message: 'current-change 事件触发', + message: `current-change 事件触发，当前页：${currentPage.value}`, status: 'info' }) }examples/sites/demos/pc/app/pager/pager-event.vue (1)
81-86: Maintain consistency with Composition API demo.For consistency with the Composition API demo, consider enhancing the event feedback message.
dataChangeTrigger() { TinyModal.message({ - message: 'current-change 事件触发', + message: `current-change 事件触发，当前页：${this.currentPage}`, status: 'info' }) }examples/sites/demos/pc/app/pager/webdoc/pager.js (1)
231-238: Update English documentation for change-compat feature.The Chinese documentation has been updated to explain the new
changeCompatfeature, but the English documentation is missing this information.Consider adding the following to the English documentation:
-'en-US': '<p> <code>current-change</code> Triggered when the current page number is switched.</p>\n' +'en-US': `<p> + The <code>current-change</code> event is triggered when the current page changes.<br/> + The <code>size-change</code> event is triggered when the page size changes.<br/> + The <code>prev-click</code> event is triggered when clicking the previous page button, and <code>next-click</code> for the next page button.<br/> + When changing the page size on the last page, both <code>current-change</code> and <code>size-change</code> events are triggered. If both events call the same function (e.g., fetching data), debouncing should be implemented.<br/> + By default, manually changing <code>current-page</code> or <code>page-size</code> values does not trigger corresponding change events. Set <code>change-compat</code> to <code>true</code> to enable event triggering. +</p>`examples/sites/demos/apis/pager.js (1)
19-33: Enhance the property description to specify affected events.The description should explicitly list which change events are controlled by this property (e.g.,
current-change,size-change, etc.) to provide better clarity for developers.desc: { - 'zh-CN': '手动改变值时，是否触发对应的change事件', - 'en-US': 'When the value is manually changed, whether the corresponding change event is triggered' + 'zh-CN': '手动改变值时，是否触发对应的change事件 (current-change, size-change, update:current-page)', + 'en-US': 'When the value is manually changed, whether to trigger the corresponding change events (current-change, size-change, update:current-page)' },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
examples/sites/demos/apis/pager.js(1 hunks)examples/sites/demos/pc/app/pager/pager-event-composition-api.vue(3 hunks)examples/sites/demos/pc/app/pager/pager-event.spec.ts(2 hunks)examples/sites/demos/pc/app/pager/pager-event.vue(4 hunks)examples/sites/demos/pc/app/pager/webdoc/pager.js(1 hunks)packages/renderless/src/pager/index.ts(5 hunks)packages/renderless/src/pager/vue.ts(4 hunks)packages/vue/src/pager/src/index.ts(1 hunks)packages/vue/src/pager/src/mobile-first.vue(1 hunks)packages/vue/src/pager/src/pc.vue(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/renderless/src/pager/index.ts
[error] 75-75: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (8)
examples/sites/demos/pc/app/pager/pager-event-composition-api.vue (1)
12-20: LGTM! Clear demonstration of the change-compat feature.The implementation effectively shows how the change-compat property affects event triggering when current-page is changed programmatically.
examples/sites/demos/pc/app/pager/pager-event.spec.ts (1)
35-36: LGTM! Good test coverage for the new feature.The test properly verifies the change-compat functionality by checking the event trigger message.
packages/renderless/src/pager/vue.ts (3)
16-16: LGTM! New import added for computed page size.The addition of
computedInternalPageSizealigns with the refactoring to make page size handling more reactive.
132-132: Enhance event handling with props context.Good improvements to event handling:
emitChangeandwatchInternalCurrentPagenow receivepropsfor better contextwatchInternalPageSizereplaceswatchPageSizesfor more consistent naming- Watch setup properly configured for the new reactive system
Also applies to: 136-137, 141-142, 148-148
83-83: Verify the computed property implementation.The
internalPageSizestate is now derived from a computed property instead of direct assignment. This change improves reactivity but requires verification of the computation logic.Run this script to check the implementation:
✅ Verification successful
The computed property implementation is correct and follows best practices
The implementation properly handles type conversion, default values, and is correctly wrapped as a Vue computed property. The conversion from direct assignment to computed property maintains proper reactivity while adding proper memoization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the computedInternalPageSize implementation ast-grep --pattern 'computedInternalPageSize({ state, props }) { $$$ }'Length of output: 75
Script:
#!/bin/bash # First, find where computedInternalPageSize is defined rg "computedInternalPageSize" -A 5Length of output: 1895
packages/vue/src/pager/src/pc.vue (1)
210-211: LGTM! New prop added for change event compatibility.The
changeCompatprop is properly added alongside existing props, maintaining consistent structure.packages/vue/src/pager/src/mobile-first.vue (1)
57-58: LGTM! Consistent prop addition across components.The
changeCompatprop is added to maintain feature parity with the PC version.packages/renderless/src/pager/index.ts (1)
Line range hint
71-496: Verify backward compatibility of thechangeCompatfeature.The addition of
changeCompatchanges the event emission behavior. Please verify:
- Default behavior (
changeCompat: false) matches the previous implementation- Impact on existing applications using the pager component
Run this script to find existing usages of affected events:
✅ Verification successful
changeCompatimplementation maintains backward compatibilityThe implementation is safe because:
- It's an opt-in feature (default: false)
- Default behavior maintains emission of both events (
update:current-pageandcurrent-change)- Existing applications using either event pattern will continue to work as expected
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of pager events that might be affected echo "Searching for current-change event usage:" rg -A 2 "@current-change" echo "Searching for size-change event usage:" rg -A 2 "@size-change" echo "Searching for update:current-page usage:" rg -A 2 "update:current-page"Length of output: 20888
| @@ -17,7 +17,7 @@ test('事件', async ({ page }) => { | |||
| const sizeSelect = page.locator('.tiny-pager__selector') | |||
|
|
|||
| await sizeChange.click() | |||
| await sizeSelect.nth(3).getByText('20').click() | |||
| await sizeSelect.nth(4).getByText('20').click() | |||
There was a problem hiding this comment.
💡 Codebase verification
Test index for page size selection appears incorrect
The test uses index 4 to select size "20", but according to the pager component's default configuration, "20" should be at index 1 in the pageSizes array [10, 20, 30, 40, 50, 100]. This mismatch could cause test failures or unexpected behavior.
- Line 20:
await sizeSelect.nth(4).getByText('20').click() - Line 31:
await sizeSelect.nth(4).getByText('20').click()
🔗 Analysis chain
Verify the page size selector indices.
The change from index 3 to 4 for size selection needs verification. Let's confirm this change is intentional and matches the actual dropdown options.
Also applies to: 31-31
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for pageSizes array definition or size options
ast-grep --pattern 'pageSizes = [$_]' || rg 'pageSizes.*\[.*\]'
Length of output: 39
Script:
#!/bin/bash
# Search for size-related patterns in test files
rg -A 3 "size.*(\[|\{|=)" --type ts
# Also search for specific size values that might indicate page size options
rg "\b(10|20|30|40|50)\b.*size" --type ts
Length of output: 75037
Script:
#!/bin/bash
# Search for the specific test file content
cat examples/sites/demos/pc/app/pager/pager-event.spec.ts
# Also search for page size related configurations in pager component
rg "pageSize|pageSizes" --type ts packages/vue/src/pager/
Length of output: 1713
Script:
#!/bin/bash
# View the pager component implementation
cat packages/vue/src/pager/src/index.ts
Length of output: 1742
| ({ state, emit, props }: Pick<IPagerRenderlessParams, 'state' | 'emit' | 'props'>) => | ||
| (currentPage: number): void => { | ||
| const value = String(currentPage) | ||
|
|
||
| if (state.jumperValue !== value) { | ||
| state.jumperValue = value | ||
| } | ||
| emit('update:current-page', currentPage) | ||
| emit('current-change', currentPage) | ||
| state.lastEmittedPage = -1 | ||
| } | ||
|
|
||
| export const watchPageSizes = | ||
| ({ state, props }: Pick<IPagerRenderlessParams, 'props' | 'state'>) => | ||
| (newVal: number[]): void => { | ||
| if (Array.isArray(newVal)) { | ||
| state.internalPageSize = newVal.includes(props.pageSize) ? props.pageSize : newVal[0] | ||
| if (props.changeCompat) { | ||
| emit('update:current-page', currentPage) | ||
| emit('current-change', currentPage) | ||
| } | ||
| state.lastEmittedPage = -1 |
There was a problem hiding this comment.
Fix inconsistent changeCompat logic across event emitters.
The changeCompat check is implemented inconsistently:
- In watchers (
watchInternalCurrentPage,watchInternalPageSize): Events are emitted whenchangeCompatistrue - In
emitChange: Events are emitted whenchangeCompatisfalse
This inconsistency could lead to unexpected behavior.
Choose one consistent approach. Recommendation: Emit events when changeCompat is false (immediate mode) across all handlers:
export const watchInternalCurrentPage =
({ state, emit, props }: Pick<IPagerRenderlessParams, 'state' | 'emit' | 'props'>) =>
(currentPage: number): void => {
const value = String(currentPage)
if (state.jumperValue !== value) {
state.jumperValue = value
}
- if (props.changeCompat) {
+ if (!props.changeCompat) {
emit('update:current-page', currentPage)
emit('current-change', currentPage)
}
state.lastEmittedPage = -1
}
export const watchInternalPageSize =
({ props, emit }: Pick<IPagerRenderlessParams, 'props' | 'emit'>) =>
(pageSize: number): void => {
- if (props.changeCompat) {
+ if (!props.changeCompat) {
emit('size-change', pageSize)
}
}Also applies to: 156-161, 489-496
| export const computedInternalPageSize = | ||
| ({ props }: Pick<IPagerRenderlessParams, 'props'>) => | ||
| (): number => { | ||
| const { pageSize, pageSizes } = props | ||
| let internalPageSize = isNaN(pageSize) ? 10 : Number(pageSize) | ||
| if (Array.isArray(pageSizes)) { | ||
| internalPageSize = pageSizes.includes(pageSize) ? pageSize : pageSizes[0] | ||
| } | ||
| return internalPageSize | ||
| } |
There was a problem hiding this comment.
Fix unsafe isNaN usage and enhance type safety.
The function uses the unsafe global isNaN which attempts type coercion. Also, the type checking could be more robust.
export const computedInternalPageSize =
({ props }: Pick<IPagerRenderlessParams, 'props'>) =>
(): number => {
const { pageSize, pageSizes } = props
- let internalPageSize = isNaN(pageSize) ? 10 : Number(pageSize)
+ let internalPageSize = typeof pageSize !== 'number' || Number.isNaN(pageSize) ? 10 : pageSize
if (Array.isArray(pageSizes)) {
- internalPageSize = pageSizes.includes(pageSize) ? pageSize : pageSizes[0]
+ internalPageSize = pageSizes.includes(internalPageSize) ? internalPageSize : pageSizes[0]
}
return internalPageSize
}📝 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.
| export const computedInternalPageSize = | |
| ({ props }: Pick<IPagerRenderlessParams, 'props'>) => | |
| (): number => { | |
| const { pageSize, pageSizes } = props | |
| let internalPageSize = isNaN(pageSize) ? 10 : Number(pageSize) | |
| if (Array.isArray(pageSizes)) { | |
| internalPageSize = pageSizes.includes(pageSize) ? pageSize : pageSizes[0] | |
| } | |
| return internalPageSize | |
| } | |
| export const computedInternalPageSize = | |
| ({ props }: Pick<IPagerRenderlessParams, 'props'>) => | |
| (): number => { | |
| const { pageSize, pageSizes } = props | |
| let internalPageSize = typeof pageSize !== 'number' || Number.isNaN(pageSize) ? 10 : pageSize | |
| if (Array.isArray(pageSizes)) { | |
| internalPageSize = pageSizes.includes(internalPageSize) ? internalPageSize : pageSizes[0] | |
| } | |
| return internalPageSize | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 75-75: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/renderless/src/pager/vue.ts (1)
132-132:⚠️ Potential issueFix inconsistent
changeCompatlogic across event emitters.The event emission logic is inconsistent:
- In watchers: Events are emitted when
changeCompatistrue- In
emitChange: Events are emitted whenchangeCompatisfalseThis inconsistency could lead to unexpected behavior.
Also applies to: 136-137, 141-142
packages/renderless/src/pager/index.ts (1)
477-486:⚠️ Potential issueFix unsafe
isNaNusage and enhance type safety.The function uses the unsafe global
isNaNwhich attempts type coercion. Also, the type checking could be more robust.🧰 Tools
🪛 Biome (1.9.4)
[error] 483-483: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
🧹 Nitpick comments (5)
examples/sites/demos/pc/webdoc/faq.md (2)
42-68: LGTM! Well-structured example with proper implementation.The code example effectively demonstrates the
change-compatfeature with a complete, working implementation.Consider adding comments to highlight key aspects of the implementation:
<template> <div> + <!-- Button to demonstrate programmatic page change --> <tiny-button @click="currentPage = 1">改变current-page</tiny-button> + <!-- Pager with change-compat enabled to trigger events on programmatic changes --> <tiny-pager :total="100" :current-page="currentPage" mode="number" change-compat @current-change="dataChangeTrigger" ></tiny-pager> </div> </template>
70-74: Consider using more specific CSS selectors.The current styling might affect other buttons with the same class. Consider using a more specific selector to avoid potential conflicts.
<style scoped> - .tiny-button { + .tiny-button[class] { margin-bottom: 12px; } </style>examples/sites/demos/pc/webdoc/faq-en.md (2)
37-40: Enhance the documentation with additional details.Consider adding the following information to make the documentation more comprehensive:
- Specify that
change-compatis a boolean property with a default value offalse- List all events that are affected by this property
- Add a note about potential performance implications when enabled
42-75: Enhance the code example for better clarity.The example effectively demonstrates the feature but could be improved:
- Add comments explaining key parts of the implementation
- Show a side-by-side comparison with and without
change-compat- Use more descriptive variable and message text
Here's a suggested improvement:
<template> <div> - <tiny-button @click="currentPage = 1">change current-page</tiny-button> + <!-- Example 1: With change-compat --> + <tiny-button @click="currentPage = 1">Set Page to 1 (with change-compat)</tiny-button> <tiny-pager :total="100" :current-page="currentPage" mode="number" change-compat @current-change="dataChangeTrigger" ></tiny-pager> + + <!-- Example 2: Without change-compat --> + <tiny-button @click="currentPage2 = 1">Set Page to 1 (without change-compat)</tiny-button> + <tiny-pager + :total="100" + :current-page="currentPage2" + mode="number" + @current-change="dataChangeTrigger2" + ></tiny-pager> </div> </template> <script setup> import { ref } from 'vue' import { TinyPager, TinyModal, TinyButton } from '@opentiny/vue' // Initialize page numbers const currentPage = ref(10) + const currentPage2 = ref(10) const dataChangeTrigger = () => { TinyModal.message({ - message: 'current-change is triggered', + message: 'Event triggered (with change-compat): Page changed programmatically', status: 'info' }) } + + const dataChangeTrigger2 = () => { + TinyModal.message({ + message: 'Event triggered (without change-compat): Only on user interaction', + status: 'info' + }) + } </script>packages/renderless/src/pager/vue.ts (1)
149-161: Improve watch handlers to avoid assignments in expressions.The watch handlers use assignments in expressions, which can be confusing and harder to maintain.
Apply this diff to improve readability:
watch( () => props.pageSize, - () => (state.internalPageSize = api.getInternalPageSize()) + () => { + state.internalPageSize = api.getInternalPageSize() + } ) watch( () => props.pageSizes, - () => (state.internalPageSize = api.getInternalPageSize()) + () => { + state.internalPageSize = api.getInternalPageSize() + } )🧰 Tools
🪛 Biome (1.9.4)
[error] 156-156: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 160-160: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
examples/sites/demos/pc/webdoc/faq-en.md(1 hunks)examples/sites/demos/pc/webdoc/faq.md(1 hunks)packages/renderless/src/pager/index.ts(4 hunks)packages/renderless/src/pager/vue.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/renderless/src/pager/vue.ts
[error] 156-156: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 160-160: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
packages/renderless/src/pager/index.ts
[error] 483-483: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: PR Unit Test
- GitHub Check: PR E2E Test (pnpm test:e2e3)
🔇 Additional comments (2)
examples/sites/demos/pc/webdoc/faq.md (1)
37-40: LGTM! Clear and well-structured explanation.The explanation effectively communicates the purpose of
change-compat, its default behavior, and how to use it. The use of the Pager component as an example makes it easy to understand the practical application.packages/renderless/src/pager/vue.ts (1)
16-16: LGTM! Improved initialization ofinternalPageSizeThe change from direct prop access to using a dedicated function improves the initialization logic and makes it more maintainable.
Also applies to: 83-83, 146-146
…nge event when value change
PR
新增change-compat特性，用于控制在响应式值变更时是否触发对应的change事件
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
change-compatproperty to the Pager component.current-pageproperty.Bug Fixes
Documentation
change-compatproperty behavior.Improvements