feat: update block filters UI in materials #914
Conversation
github-actions
bot
added
enhancement
WalkthroughThe changes introduce a conditional rendering mechanism in the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (3)
54-61: Consider adding type validation for filter IDsThe state initialization looks good but could be more robust with validation.
Consider this enhancement:
const state = reactive({ checkGroup: props.filters.reduce( (result, filter) => ({ ...result, - [filter.id]: [] + [filter.id]: filter.id ? [] : console.warn(`Filter missing ID: ${filter.name}`) }), {} ) })
63-74: Translate Chinese comment and enhance documentationThe implementation is correct, but the comment should be in English for consistency.
Consider replacing the comment with:
- // 这里重新计算selectOptions的原因：tiny-option的value属性如果是一个对象，那么此对象内部需要有value属性 + // Compute selectOptions to ensure each option object contains a 'value' property, + // which is required by tiny-option when its value prop is an object
77-89: Consider using type guards for better type safetyThe type checking logic works but could be more explicit.
Consider this enhancement:
- if (typeof checked.at(0) === 'string') { + // Type guard for checkbox group values + if (Array.isArray(checked) && typeof checked[0] === 'string') {packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
Line range hint
1-1: Consider adding documentation for the new filter behavior.Since this is a UI enhancement that changes how filters behave, it would be beneficial to:
- Update the component's documentation to explain the
usingSelectproperty- Add comments explaining when to use select vs. checkbox group filters
- Update any relevant UI/UX guidelines
Would you like me to help draft the documentation for these changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue(4 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue(1 hunks)
🔇 Additional comments (3)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (2)
7-13: LGTM: Checkbox group implementation looks correct
The conditional rendering and data binding for the checkbox group is properly implemented.
14-28: Verify the UX of hover-expand behavior
The select component implementation looks good, but the hover-expand prop might affect usability with multiple selections. Please verify that:
- The dropdown remains open while making multiple selections
- The hover behavior doesn't interfere with selection of multiple items
packages/plugins/materials/src/meta/block/src/BlockGroupPanel.vue (1)
100-101: LGTM! The filter configuration changes align with the UI enhancement goals.
The addition of usingSelect: true to the author and tag filters is a clean way to implement the UI enhancement, allowing these filters to render as select components instead of checkbox groups. This change will help manage large numbers of filter options more effectively.
Also applies to: 106-107
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (4)
7-27: Consider enhancing accessibility for the select component.While the implementation is functionally correct, consider adding ARIA labels and
aria-describedbyattributes to improve accessibility for screen readers.<tiny-select v-else v-model="state.checkGroup[filter.id]" size="mini" multiple + :aria-label="filter.name" + :aria-describedby="`${filter.id}-description`" @change="getFilters($event, filter.id)" >
62-73: Translate Chinese comment to English for better maintainability.The Chinese comment explains an important implementation detail about the value property requirement for tiny-option.
- // 这里重新计算selectOptions的原因：tiny-option的value属性如果是一个对象，那么此对象内部需要有value属性 + // Recalculating selectOptions because when tiny-option's value is an object, that object must have a 'value' property
76-89: Enhance type checking in getFilters method.The current type checking using
typeof checked.at(0)could be more robust. Consider checking for object type explicitly and handling empty arrays.- if (typeof checked.at(0) === 'string') { + if (!checked.length) { + return emit('search', null, filters) + } + if (!checked.some(item => typeof item === 'object')) {
Line range hint
44-49: Add prop validation for filters array items.The
filtersprop type is defined as Array but lacks validation for the required properties (id,name,children,usingSelect).filters: { type: Array, - default: () => [] + default: () => [], + validator(value) { + return value.every(filter => + typeof filter.id === 'string' && + typeof filter.name === 'string' && + Array.isArray(filter.children) && + typeof filter.usingSelect === 'boolean' + ) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/plugins/materials/src/meta/block/src/BlockGroupArrange.vue(1 hunks)packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/plugins/materials/src/meta/block/src/BlockGroupArrange.vue
🔇 Additional comments (1)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (1)
141-143: Browser compatibility concern with :has() selector.
The CSS :has() selector might not be supported in all browsers. Consider using a class-based approach instead.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (4)
53-60: Simplify state initializationThe current implementation using reduce could be simplified for better readability.
Consider this alternative:
- checkGroup: props.filters.reduce( - (result, filter) => ({ - ...result, - [filter.id]: [] - }), - {} - ) + checkGroup: Object.fromEntries(props.filters.map(filter => [filter.id, []]))
62-62: Translate Chinese comment to EnglishFor better international collaboration, comments should be in English.
- // 这里重新计算selectOptions的原因：tiny-option的value属性如果是一个对象，那么此对象内部需要有value属性 + // Recalculating selectOptions because tiny-option's value property requires an internal value property when it's an object
79-87: Improve type checking robustnessThe current type checking using
typeof checked.at(0) === 'string'could be more robust.Consider this improvement:
- if (typeof checked.at(0) === 'string') { + if (Array.isArray(checked) && checked.length > 0 && typeof checked[0] === 'string') {
141-143: Remove duplicate selector in deep selectorThe selector
.tiny-select.tiny-selectcontains a duplicate class name.- :deep(.tiny-select.tiny-select .tiny-select__tags .tiny-tag) { + :deep(.tiny-select .tiny-select__tags .tiny-tag) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue(4 hunks)
🔇 Additional comments (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (2)
17-17: Verify size prop consistency across components
The size="mini" prop is used here. Let's ensure this matches the design system guidelines.
✅ Verification successful
Size prop "mini" is consistently used across components
The size="mini" prop is used consistently across multiple components in the codebase, particularly in similar UI contexts like form inputs, radio groups, and selectors. The usage aligns with the following components:
- NumberConfigurator
- FileSelector
- SettingPanel
- BlockGroupFilters
- NumericSelect
- BlockGroup
The codebase also shows a consistent pattern of size values, mainly using "mini" and "small" for different UI contexts, where "mini" is typically used for form controls and filters.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check size prop usage across components
rg 'size="mini"' --type vue
Length of output: 57
Script:
#!/bin/bash
# Check size prop usage across components with ripgrep, without file type filter
rg 'size="mini"'
Length of output: 784
Script:
#!/bin/bash
# Check for other size prop values in the codebase
rg 'size="[^"]+"'
Length of output: 3958
Script:
#!/bin/bash
# Find component files to understand the component structure
fd -e vue
Length of output: 16774
10-10: Inconsistent event handler parameter passing between components
The checkbox group passes filter children to getFilters, while the select component doesn't. This inconsistency could lead to maintenance issues.
Let's verify if this difference is intentional by checking other similar components:
Also applies to: 19-19
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (4)
7-27: Consider unifying event handler signatures.The event handlers for checkbox and select components have different parameter patterns:
- Checkbox:
@change="getFilters($event, filter.id, filter.children)"- Select:
@change="getFilters($event, filter.id)"This inconsistency could make maintenance more challenging.
Consider unifying the handlers by moving the children lookup into the
getFiltersmethod:- @change="getFilters($event, filter.id, filter.children)" + @change="getFilters($event, filter.id)"Then update the method to get children from props when needed.
14-27: Enhance accessibility for the select component.The select component could benefit from additional accessibility attributes.
Add aria labels and placeholder text:
<tiny-select v-else v-model="state.checkGroup[filter.id]" size="mini" multiple + :aria-label="filter.name" + :placeholder="'Select ' + filter.name" @change="getFilters($event, filter.id)" >
62-73: Translate Chinese comment to English for better maintainability.The Chinese comment explains an important implementation detail about tiny-option's value property requirements.
Replace the Chinese comment with English:
- // 这里重新计算selectOptions的原因：tiny-option的value属性如果是一个对象，那么此对象内部需要有value属性 + // Recomputing selectOptions because tiny-option's value property requires an object with an internal value property
141-146: Avoid using !important in CSS rules.The
!importantdeclaration in max-width calculation could make future style overrides difficult.Consider increasing selector specificity instead:
- :deep(.tiny-select.tiny-select .tiny-select__tags) { - max-width: calc(100% - 24px) !important; + :deep(.tiny-select.tiny-select:not(.is-disabled) .tiny-select__tags) { + max-width: calc(100% - 24px);
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
在物料插件里面的添加区块页面中，原来的区块筛选器是checkbox group的形式。如果checkbox数量很多，则会显示得很凌乱。现优化成select组件
What is the current behavior?
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Summary by CodeRabbit
New Features
usingSelectto filters, allowing for a selection interface on specific filters.Bug Fixes
Style