feat: support checking all blocks in material add block panel #923
Conversation
WalkthroughThe changes introduce a new 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
enhancement
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
packages/common/component/SelectAll.vue (3)
12-19: Add TypeScript type definitions for array propsThe
itemsandselectedprops would benefit from explicit type definitions for better type safety and developer experience.items: { type: Array, + type: Array as PropType<unknown[]>, default: () => [] }, selected: { type: Array, + type: Array as PropType<unknown[]>, default: () => [] },
1-42: Add unit tests and documentation for the SelectAll componentThe component looks well-implemented, but it's missing:
- Unit tests to verify the component's behavior
- Documentation explaining props, events, and usage examples
Would you like me to help create:
- Unit tests covering key functionality?
- Documentation in JSDoc format?
28-39: Consider adding error handling for edge casesThe
selectedAllcomputed property's setter should handle edge cases:
- When items array is empty
- When items contain duplicates
const selectedAll = computed({ get() { - return props.items.length > 0 && props.items.length === props.selected.length + return props.items.length > 0 && props.selected.length === new Set(props.items).size }, set(value) { + if (props.items.length === 0) return if (value) { - emit('selectAll', props.items) + emit('selectAll', [...new Set(props.items)]) } else { emit('deselectAll') } } })packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue (2)
4-11: Add accessibility attributes to SelectAll componentEnhance accessibility by adding an aria-label to the SelectAll component.
<select-all v-if="state.displayType === 'grid'" class="block-select-all" + aria-label="Select all blocks" :items="state.blockList" :selected="selectedBlockArray" @select-all="checkAll" @deselect-all="cancelCheckAll" ></select-all>
9-10: Consider normalizing event names for consistencyThe events
select-all/deselect-allfrom SelectAll component don't match the handler namescheckAll/cancelCheckAll. Consider normalizing these for better maintainability.Either update the SelectAll component:
- @select-all="checkAll" - @deselect-all="cancelCheckAll" + @checkAll="checkAll" + @cancelCheckAll="cancelCheckAll"Or update the handlers to match the events:
- @select-all="checkAll" - @deselect-all="cancelCheckAll" + @select-all="handleSelectAll" + @deselect-all="handleDeselectAll"packages/plugins/materials/src/meta/block/src/BlockList.vue (1)
19-20: Use consistent event handler binding patternThe event handlers use different binding patterns - arrow function for
checkAllbut direct method reference forcancelCheckAll. Consider using the same pattern for both for consistency.- @checkAll="(items) => checkAll(items)" - @cancelCheckAll="cancelCheckAll" + @checkAll="checkAll" + @cancelCheckAll="cancelCheckAll"packages/plugins/block/src/composable/useBlock.js (1)
702-704: Consider defensive copying for selectedBlockArrayThe direct assignment of blockList could lead to unexpected behavior if the source array is mutated externally. Consider creating a new array to prevent external mutations.
- selectedBlockArray.value = blockList + selectedBlockArray.value = [...blockList]packages/common/component/PluginBlockList.vue (1)
374-380: Consider adding type safety to the handler methods.While the implementation is correct, consider adding TypeScript or JSDoc type annotations for better maintainability and IDE support.
- const handleSelectAll = (items) => { + /** + * @param {Array} items - The items to be selected + */ + const handleSelectAll = (items) => { emit('checkAll', items) } - const handleDeselectAll = () => { + /** + * Handles deselection of all items + */ + const handleDeselectAll = () => { emit('cancelCheckAll') }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
packages/common/component/PluginBlockList.vue(7 hunks)packages/common/component/SelectAll.vue(1 hunks)packages/common/component/index.js(1 hunks)packages/plugins/block/src/composable/useBlock.js(2 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(2 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue(7 hunks)packages/plugins/materials/src/meta/block/src/BlockList.vue(4 hunks)packages/theme/base/src/component-common.less(1 hunks)
🔇 Additional comments (12)
packages/common/component/index.js (1)
38-38: LGTM!
The export follows the established pattern in the file.
packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue (1)
29-30: Verify event handler implementation in BlockList component
The BlockList component now receives new events @checkAll and @cancelCheckAll. Let's verify their implementation.
✅ Verification successful
Let me gather more information to verify the implementation of these event handlers.
Event handlers are properly implemented in BlockList component
The verification shows that both checkAll and cancelCheckAll events are:
- Correctly declared in the component's
emitsoption - Properly implemented as methods that emit the corresponding events
- Appropriately bound in the template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for checkAll/cancelCheckAll event implementations in BlockList
ast-grep --pattern 'emit("checkAll", $$$)' || ast-grep --pattern 'emit("cancelCheckAll", $$$)'
# Search for event declarations
rg -A 5 'emits.*checkAll.*cancelCheckAll'
Length of output: 1321
Script:
#!/bin/bash
# Search for checkAll/cancelCheckAll method implementations in BlockList
ast-grep --pattern 'function checkAll() {
$$$
}' || ast-grep --pattern 'const checkAll = () => {
$$$
}'
ast-grep --pattern 'function cancelCheckAll() {
$$$
}' || ast-grep --pattern 'const cancelCheckAll = () => {
$$$
}'
# Search for any usage of these events in the file
rg -A 3 'checkAll|cancelCheckAll' packages/plugins/materials/src/meta/block/src/BlockList.vue
Length of output: 794
packages/plugins/materials/src/meta/block/src/BlockList.vue (1)
Line range hint 201-219: LGTM: Clean implementation of selection methods
The implementation is clean and follows Vue 3 composition API patterns correctly.
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (2)
80-80: LGTM: Proper import of new functionality
The cancelCheckAll method is correctly destructured from useBlock composable.
158-160: LGTM: Good error handling practice
Excellent use of the finally block to ensure selection state is cleaned up regardless of whether the block addition succeeds or fails.
packages/plugins/block/src/composable/useBlock.js (2)
706-708: LGTM: Clean implementation of cancelCheckAll
The implementation correctly resets the selection state by assigning a new empty array.
748-749: LGTM: Proper export of new functionality
The new methods are correctly exported from the composable.
packages/theme/base/src/component-common.less (1)
524-525: LGTM: Checkbox styling supports the new select-all feature.
The added styles properly handle both checked and indeterminate states of checkboxes using theme variables, maintaining consistency with the design system.
packages/common/component/PluginBlockList.vue (4)
3-11: LGTM: SelectAll component integration is well-implemented.
The component is properly integrated with:
- Conditional rendering based on showCheckbox prop
- Correct prop bindings for items and selected state
- Proper event handling
154-154: LGTM: Component registration follows best practices.
The SelectAll component is properly imported and registered in the components section.
Also applies to: 172-173
247-247: LGTM: Events are properly declared.
The emits array correctly includes the new events 'checkAll' and 'cancelCheckAll'.
469-473: LGTM: Checkbox column styling is well-structured.
The styling:
- Uses flexbox for proper alignment
- Maintains consistency with existing styles
- Is scoped appropriately
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/common/component/PluginBlockList.vue (1)
368-374: Simplify the event handler implementation.The event handler can be simplified by using object destructuring and early return pattern.
Consider this more concise implementation:
-const handleSelectAll = (items) => { - if (Array.isArray(items)) { - emit('checkAll', items) - } else { - emit('cancelCheckAll') - } -} +const handleSelectAll = (items) => { + if (!Array.isArray(items)) { + return emit('cancelCheckAll') + } + emit('checkAll', items) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
packages/common/component/PluginBlockList.vue(7 hunks)packages/common/component/SelectAll.vue(1 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/common/component/SelectAll.vue
- packages/plugins/materials/src/meta/block/src/BlockGroupTransfer.vue
🔇 Additional comments (5)
packages/common/component/PluginBlockList.vue (5)
148-148: LGTM: Component registration is properly implemented.
The SelectAll component is correctly imported and registered in the components section.
Also applies to: 166-167
241-241: LGTM: Event emits are properly declared.
The new events 'checkAll' and 'cancelCheckAll' are correctly added to the emits array.
4-4: Simplify the event binding syntax.
The event binding can be simplified as noted in the previous review.
Apply this change:
-<select-all :allItems="data" :selected="checked" :hidden-label="true" @select-all="(items) => handleSelectAll(items)"></select-all>
+<select-all :allItems="data" :selected="checked" :hidden-label="true" @select-all="handleSelectAll"></select-all>462-466: LGTM: Checkbox styling is properly implemented.
The checkbox styling uses flex layout appropriately for alignment.
4-4: Verify the SelectAll component's contract.
Ensure that the SelectAll component's contract (props and events) aligns with its usage here.
✅ Verification successful
Let me gather more specific information about the SelectAll component's implementation.
Based on the SelectAll component's implementation, I can now provide the final verification response:
The SelectAll component's contract matches its usage correctly
The component implementation shows that all props and events used in PluginBlockList.vue are properly defined:
- Props:
allItems: Array (matchesdata)selected: Array (matcheschecked)hiddenLabel: Boolean (matcheshidden-label)
- Events:
selectAllevent is properly defined and emitted (matches@select-all)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the SelectAll component's implementation
# Look for the SelectAll component's implementation to verify its contract
# Search for the SelectAll component file
echo "Searching for SelectAll component file..."
fd -e vue -e js SelectAll
# Search for the component's props and emits
echo "Checking component's contract..."
ast-grep --pattern 'defineProps({
$$$
})'
ast-grep --pattern 'defineEmits([
$$$
])'
Length of output: 330
Script:
#!/bin/bash
# Get the content of the SelectAll component
echo "Content of SelectAll.vue:"
cat packages/common/component/SelectAll.vue
# Search for any usage patterns of SelectAll in the codebase
echo -e "\nUsage patterns of SelectAll:"
rg -A 2 "<select-all" --type vue
Length of output: 1185
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
物料插件-区块分组-添加区块，区块列表没有全选功能
改动点：
SelectAll组件到common包What is the current behavior?
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Select Allcomponent for bulk selection in the block list.Bug Fixes
addBlocksmethod to ensure selection state resets after operations.Style
Documentation