refactor(transfer): [transfer] refactor transfer by checkbox #2396
Conversation
WalkthroughThe changes in this pull request involve modifications to the titles displayed in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
|
[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: 2
🧹 Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/transfer/custom-titles.spec.ts (1)
12-14: LGTM! Consider extracting expected texts as constants.The test changes correctly reflect the new, more concise titles while maintaining proper validation of both title text and selection counts.
Consider extracting the expected texts as constants for better maintainability:
+ const LEFT_PANEL_TITLE = '数据列表' + const RIGHT_PANEL_TITLE = '已选列表' - await expect(leftPanel.locator('.tiny-checkbox__label').first()).toHaveText(/数据列表/) + await expect(leftPanel.locator('.tiny-checkbox__label').first()).toHaveText(new RegExp(LEFT_PANEL_TITLE)) await expect(leftPanel.locator('.tiny-checkbox__label').first()).toHaveText(/未勾选 \/ 共14/) - await expect(rightPanel.locator('.tiny-checkbox__label').first()).toHaveText(/已选列表/) + await expect(rightPanel.locator('.tiny-checkbox__label').first()).toHaveText(new RegExp(RIGHT_PANEL_TITLE))packages/vue/src/transfer-panel/src/pc.vue (1)
59-59: Consider using a more descriptive event handler name.The event handler name 'checkedEvent' could be more descriptive.
- @change="checkedEvent(item[state.keyProp], item[state.disabledProp])" + @change="handleItemCheckChange(item[state.keyProp], item[state.disabledProp])"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/transfer/custom-titles-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/transfer/custom-titles.spec.ts (1 hunks)
- examples/sites/demos/pc/app/transfer/custom-titles.vue (1 hunks)
- packages/theme/src/transfer/index.less (0 hunks)
- packages/vue/src/transfer-panel/src/pc.vue (1 hunks)
💤 Files with no reviewable changes (1)
- packages/theme/src/transfer/index.less
🔇 Additional comments (3)
examples/sites/demos/pc/app/transfer/custom-titles-composition-api.vue (1)
5-5: LGTM! The simplified titles improve UI conciseness.The change from ['所有数据列表', '已选数据列表'] to ['数据列表', '已选列表'] makes the UI labels more concise while maintaining clarity.
Let's verify the consistency of these title changes across other transfer-related files:
✅ Verification successful
Title changes are consistently applied across transfer components
The verification shows that both transfer components using custom titles (
custom-titles-composition-api.vueandcustom-titles.vue) have been updated to use the new concise titles ['数据列表', '已选列表']. No instances of the old titles ['所有数据列表', '已选数据列表'] were found, confirming consistent implementation across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent title usage across transfer components # Expected: All transfer components should use the new concise titles # Search for transfer title definitions rg -l "tiny-transfer.*titles" --type vue # Check the actual title content in these files rg "titles=.*['\"]数据列表['\"].*['\"]已选列表['\"]" --type vueLength of output: 164
Script:
#!/bin/bash # Search for transfer title definitions without file type restriction rg -l "tiny-transfer.*titles" # Check the actual title content in these files rg "titles=.*['\"]数据列表['\"].*['\"]已选列表['\"]" # Also search for the old titles to compare rg "titles=.*['\"]所有数据列表['\"].*['\"]已选数据列表['\"]"Length of output: 333
examples/sites/demos/pc/app/transfer/custom-titles.vue (1)
5-5: LGTM! The title changes improve conciseness.The simplified titles maintain clarity while being more concise, which aligns well with documentation best practices. The change preserves the semantic meaning while making the UI labels more user-friendly.
packages/vue/src/transfer-panel/src/pc.vue (1)
53-62: LGTM! Clean component-based checkbox implementation.The refactor to use
<tiny-checkbox>component improves code maintainability and follows Vue.js best practices.
| :checked="state.checked.length > 0 && state.checked.indexOf(item[state.keyProp]) > -1" | ||
| @change="checkedEvent(item[state.keyProp], item[state.disabledProp])" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing checked state lookup performance.
The current implementation uses array indexOf for checking item state, which could be inefficient for large datasets.
Consider using a Set or Map for O(1) lookup:
- :checked="state.checked.length > 0 && state.checked.indexOf(item[state.keyProp]) > -1"
+ :checked="state.checkedSet.has(item[state.keyProp])"You'll need to maintain a Set alongside the array in your component's state:
const checkedSet = computed(() => new Set(state.checked))| <option-content :option="optionRender(item)"></option-content> | ||
| </span> | ||
| </label> | ||
| <option-content :option="optionRender(item)"></option-content> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider memoizing option rendering.
The direct call to optionRender in the template could cause unnecessary re-renders.
Consider memoizing the option render result:
- <option-content :option="optionRender(item)"></option-content>
+ <option-content :option="getMemoizedOption(item)"></option-content>Add a computed or cached method:
const getMemoizedOption = computed(() => {
const cache = new Map()
return (item) => {
if (!cache.has(item[state.keyProp])) {
cache.set(item[state.keyProp], optionRender(item))
}
return cache.get(item[state.keyProp])
}
})
shenjunjian
changed the title
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
Release Notes
New Features
<tiny-checkbox>component for enhanced checkbox functionality within the transfer panel.Bug Fixes
Style