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 reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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);
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue
Outdated
Show resolved
Hide resolved
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)
22-23: Consider accessibility implications of mini-sized selectThe
size="mini"attribute might affect usability for users with motor control difficulties. Consider using the default size or making it configurable based on user preferences.
67-79: Translate Chinese comment and enhance documentationThe Chinese comment should be translated to English for consistency and global accessibility. Also, consider adding JSDoc documentation for the computed property.
- // 不同的filter，值所在的字段可能是id或者name。这里把实际的值都映射到value字段 + /** + * Maps filter values to a consistent structure where the actual value + * is always in the 'value' field, regardless of whether it comes from + * id or name in the original data. + */
81-85: Add error handling to getFilters methodThe method should handle edge cases such as undefined or null values.
const getFilters = (checked, id) => { + if (!id || !Array.isArray(checked)) { + console.warn('Invalid parameters passed to getFilters'); + return; + } filters[id] = checked emit('search', null, filters) }
136-141: Avoid using !important in CSS rulesThe
max-widthrule uses!importantwhich makes styles harder to maintain and override when needed. Consider restructuring the CSS specificity instead.-:deep(.tiny-select.tiny-select .tiny-select__tags) { - max-width: calc(100% - 24px) !important; +:deep(.tiny-select.tiny-select) { + .tiny-select__tags { + max-width: calc(100% - 24px); + }
📜 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)
58-64: Consider using TypeScript for better type safety
The code relies on runtime assumptions about the data structure. Consider using TypeScript or adding prop validation as suggested in the previous review.
7-32: Verify component behavior with different data sets
Please ensure to test the component with:
- Empty filter lists
- Large datasets that might overflow the select
- Mixed usage of select and checkbox groups
- Different screen sizes for responsive behavior
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (3)
7-33: Consider extracting common props and events to improve maintainability.The checkbox group and select components share similar bindings and events. Consider extracting these into a computed property or methods to reduce duplication.
+ const commonProps = computed(() => ({ + modelValue: state.checkGroup[filter.id], + 'onUpdate:modelValue': (value) => state.checkGroup[filter.id] = value, + onChange: (event) => getFilters(event, filter.id) + })) <tiny-checkbox-group v-if="!filter.usingSelect" - v-model="state.checkGroup[filter.id]" - @change="getFilters($event, filter.id)" + v-bind="commonProps" type="checkbox" > ... <tiny-select v-else - v-model="state.checkGroup[filter.id]" - @change="getFilters($event, filter.id)" + v-bind="commonProps" size="mini" multiple is-drop-inherit-width >
82-86: Enhance error handling in getFilters method.The method should validate the checked values before updating filters and emitting the event. Consider adding error boundaries and logging.
- const getFilters = (checked, id) => { + const getFilters = (checked, id) => { + if (!id || !Array.isArray(checked)) { + console.warn('Invalid arguments passed to getFilters:', { checked, id }); + return; + } + filters[id] = checked emit('search', null, filters) }
137-142: Consider using CSS custom properties for magic numbers.The max-width calculation uses a hardcoded value. Consider extracting this to a CSS custom property for better maintainability.
+ :root { + --select-tags-padding: 24px; + } :deep(.tiny-select.tiny-select .tiny-select__tags) { - max-width: calc(100% - 24px) !important; + max-width: calc(100% - var(--select-tags-padding)) !important; .tiny-tag { background-color: var(--te-common-bg-container); } }
📜 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)
59-65: Consider using TypeScript or prop validation for better type safety.
The code relies on runtime checks and assumptions about data structure. This approach could be improved with proper type definitions.
22-22: Consider accessibility implications of mini-sized select.
The size="mini" prop might affect touch targets on mobile devices. Consider using a medium size by default and only applying mini size for desktop views.
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 (2)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (2)
19-33: Consider enhancing accessibility for the select component.While the select implementation is functionally correct, consider adding these accessibility attributes:
<tiny-select v-else v-model="state.checkGroup[filter.id]" size="mini" multiple is-drop-inherit-width + :aria-label="filter.name" + :placeholder="`Select ${filter.name}`" @change="getFilters($event, filter.id)" >
82-86: Consider adding validation in the getFilters method.The method should validate the input parameters and handle edge cases.
-const getFilters = (checked, id) => { +const getFilters = (checked, id) => { + if (!id || !Array.isArray(checked)) { + console.warn('Invalid parameters passed to getFilters'); + return; + } filters[id] = checked emit('search', null, filters) }
📜 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 (5)
packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue (5)
7-17: LGTM! Clean implementation of conditional rendering for the checkbox group.
The checkbox group implementation is well-structured with consistent value and event bindings.
59-65: Consider using TypeScript or prop validation for better type safety.
The state initialization could benefit from stronger type checking.
100-105: LGTM! Clean spacing implementation.
The spacing adjustments improve the layout and readability of the filter items.
137-142: Browser compatibility concerns with :has selector.
Consider implementing a class-based fallback strategy.
7-33: Verify the filter types using the new select component.
Let's verify that all intended filter types are properly configured to use the select component.
✅ Verification successful
Based on the search results, I can now generate the final response:
Filter types are properly configured for select component migration
The implementation correctly handles both checkbox and select modes:
- Found configuration in
BlockGroupPanel.vueshowingusingSelect: truefor the intended filter types BlockGroupFilters.vueimplements the conditional rendering logic (v-if="!filter.usingSelect") to switch between checkbox and select components- The component properly maintains the same data structure and event handling for both modes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for filter configurations to verify usingSelect property
rg -A 5 "usingSelect.*true" --type vue
# Search for filter type definitions
ast-grep --pattern 'filters: {
$$$
author: {
$$$
},
tag: {
$$$
}
$$$
}'
Length of output: 162
Script:
#!/bin/bash
# Search for filter configurations with correct file type
rg -A 5 "usingSelect.*true"
# Search for filter definitions in the component
rg -A 10 "filters.*=.*{" packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue
# Look for the setup or data section where filters might be defined
ast-grep --pattern 'setup() {
$$$
}'
# Search for any filter-related configuration
rg -B 2 -A 5 "filter" packages/plugins/materials/src/meta/block/src/BlockGroupFilters.vue
Length of output: 4059
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/theme/base/src/component-common.less (1)
20-24: Ensure consistent border-radius across tag componentsThe changes improve the visual appearance of tags by adding border-radius and standardizing the close button size. However, consider the following suggestions:
.tiny-tag { color: var(--te-common-text-weaken); background-color: var(--te-common-bg-tag); border-radius: var(--te-base-border-radius-normal); + // Add hover state for better user interaction + &:hover { + background-color: var(--te-common-bg-container); + } .tiny-tag__close { color: var(--te-common-text-primary); font-size: 12px; + // Add transition for smooth hover effect + transition: color 0.3s ease; &:hover { color: var(--te-common-text-primary); } } }
…to refactor/block-add-ui
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
usingSelectproperty to filters, enabling a selection interface for specific filters.Bug Fixes
Style