feat: table search form visible control #5121
Conversation
mynetfan
requested review from
anncwb,
vince292007 and
a team
as code owners
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
packages/effects/plugins/src/vxe-table/api.tsOops! Something went wrong! :( ESLint: 9.16.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs WalkthroughThe pull request introduces enhancements to the Vben Vxe Table component, focusing on the search functionality. Key updates include the addition of a 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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
109-123: Simplify Conditional Logic for Toolbar ToolsThe conditional assignment for
toolbarConfig.toolsworks correctly but can be made more readable.Consider extracting the condition into a descriptive variable:
const showSearchTool = gridOptions.value?.toolbarConfig?.search && !!formOptions.value; const toolbarConfig: VxeGridPropTypes.ToolbarConfig = { tools: showSearchTool ? [ { code: 'search', icon: 'vxe-icon--search', circle: true, status: isFormShow.value ? 'primary' : undefined, title: $t('common.search'), }, ] : [], };This enhances readability by making the purpose of the condition clearer.
packages/effects/plugins/src/vxe-table/types.ts (1)
22-25: LGTM: Well-documented toolbar search optionThe
ToolbarConfigOptionsinterface is well-defined with clear JSDoc comments in Chinese. Consider adding English translations for better international developer experience.playground/src/views/examples/vxe-table/form.vue (1)
88-88: Consider configuring exportConfigAn empty
exportConfigobject is added but not configured. Consider adding necessary export configuration or removing if not needed.docs/src/components/common-ui/vben-vxe-table.md (1)
168-169: Enhance search form documentation with examplesWhile the documentation explains the feature, it would be more helpful with code examples.
Consider adding a code example showing how to configure and use the search form visibility control:
// Example configuration const gridOptions = { toolbarConfig: { search: true, // Enable search form control button }, }; // Example usage with API gridApi.toggleSearchForm(); // Toggle visibility gridApi.setFormVisible(false); // Hide form
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/src/components/common-ui/vben-vxe-table.md(3 hunks)docs/src/demos/vben-vxe-table/form/index.vue(1 hunks)packages/effects/plugins/src/vxe-table/api.ts(4 hunks)packages/effects/plugins/src/vxe-table/index.ts(1 hunks)packages/effects/plugins/src/vxe-table/types.ts(4 hunks)packages/effects/plugins/src/vxe-table/use-vxe-grid.vue(5 hunks)packages/locales/src/langs/en-US/common.json(1 hunks)packages/locales/src/langs/zh-CN/common.json(1 hunks)playground/src/views/examples/vxe-table/custom-cell.vue(1 hunks)playground/src/views/examples/vxe-table/form.vue(4 hunks)
🔇 Additional comments (17)
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (4)
4-6: Approved: Imported Types from 'vxe-table'
The addition of VxeGridDefines and VxeGridPropTypes imports enhances type safety for the component.
62-62: Approved: Added 'isFormShow' to prioritized values
Including isFormShow allows the component to control the visibility of the form based on priority values.
117-117: Confirm Usage of 'status' Property
The status property is set to 'primary' when isFormShow.value is true. Ensure that 'primary' is a valid status value for styling the toolbar button in your UI library.
Check the documentation of the button component to confirm that the status prop accepts 'primary' and applies the desired styling.
335-339: Review Change from 'v-if' to 'v-show'
Switching from v-if to v-show alters how the form component is managed in the DOM.
v-ifremoves the element from the DOM when the condition is false, which resets component state.v-showtoggles the visibility but keeps the component in the DOM, preserving its state.
Ensure that:
- Keeping the form's state when hidden is intentional.
- There are no unintended side effects from preserving the form's state.
If the form should reset when hidden, consider retaining v-if.
packages/locales/src/langs/zh-CN/common.json (1)
13-13: Clarify Distinction Between 'query' and 'search'
Both "query" and "search" are translated as "搜索".
- If both keys serve different purposes, consider using distinct translations to avoid confusion.
- If they are interchangeable, one key may suffice.
Review the usage context to determine if both keys are necessary.
packages/effects/plugins/src/vxe-table/index.ts (2)
2-2: Approved: Exporting 'VxeTableGridOptions'
Exporting VxeTableGridOptions makes the type available for external use, enhancing type definitions.
5-5: Approved: Reordered Export Statements
Reordering exports improves code organization without affecting functionality.
packages/locales/src/langs/en-US/common.json (1)
13-13: Differentiate Between 'query' and 'search'
Both "query" and "search" have the value "Search".
- Determine if the two keys are intended for different contexts.
- If they have distinct meanings, consider updating one of the values to reflect this difference.
This will help maintain clarity in the application's UI text.
packages/effects/plugins/src/vxe-table/types.ts (1)
51-51: Verify impact of gridOptions type change
The change from VxeTableGridProps to VxeTableGridOptions for gridOptions type could affect existing code.
Also applies to: 60-63
✅ Verification successful
No breaking changes from gridOptions type update
The type change is safe because:
VxeTableGridOptionsextendsVxeTableGridProps, maintaining backward compatibility- All existing usages of
gridOptionsin the codebase are using the implementation type (VxeGridProps['gridOptions']) or object literals, not theVxeTableGridPropstype directly - The type is only used as an optional property with
DeepPartial, providing additional flexibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing gridOptions usage that might be affected
rg "gridOptions.*:.*VxeTableGridProps" --type ts
Length of output: 50
Script:
#!/bin/bash
# Search for any usage of gridOptions in TypeScript files
rg "gridOptions" --type ts -A 2
# Also search for VxeTableGridProps type references
rg "VxeTableGridProps" --type ts -A 2
# Search for imports or type definitions related to VxeTable grid types
rg "(VxeTableGridProps|VxeTableGridOptions)" --type ts
Length of output: 2209
playground/src/views/examples/vxe-table/custom-cell.vue (1)
85-85: Verify visual impact of disabled overflow
Setting showOverflow: false might affect how cell content is displayed, especially for the Product Name column which could contain long text.
Consider testing with various content lengths to ensure proper display.
playground/src/views/examples/vxe-table/form.vue (2)
104-111: LGTM: Comprehensive toolbar configuration
The toolbar configuration enables all necessary features including search, which aligns with the PR objective. The configuration is well-structured and includes useful additional features like export and zoom.
114-117: LGTM: Improved parameter structure
The useVbenVxeGrid call has been restructured for better readability while maintaining functionality.
packages/effects/plugins/src/vxe-table/api.ts (3)
24-24: LGTM: Default state initialization
The isFormShow property is properly initialized with a sensible default value.
85-89: LGTM: Clean implementation of setFormVisible
The method provides direct control over form visibility with a clear and concise implementation.
119-127: Address commented out grid recalculation code
The commented out grid recalculation code suggests a potential layout issue that was encountered during development.
Please verify if removing the grid recalculation affects the layout when toggling the form visibility. Consider either:
- Removing the commented code if it's not needed
- Implementing the recalculation if it's necessary for proper layout
docs/src/components/common-ui/vben-vxe-table.md (2)
220-228: LGTM: Clear API documentation
The API documentation for the new methods is clear and well-structured.
242-242: LGTM: Props documentation
The isFormShow prop is properly documented with a clear description.
|
大神，isFormShow 改成 isShowForm 写起来会不会顺畅点 个人愚见，见谅，可以无视 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/effects/plugins/src/vxe-table/types.ts (2)
22-25: LGTM! Consider adding English documentation.The interface is well-defined and properly extends the base type. Consider adding English documentation for better international developer experience.
interface ToolbarConfigOptions extends VxeGridPropTypes.ToolbarConfig { - /** 是否显示切换搜索表单的按钮 */ + /** Whether to show the search form toggle button + * 是否显示切换搜索表单的按钮 + */ search?: boolean; }
52-52: LGTM! Consider adding English documentation.The type changes are correct and maintain type safety. Consider adding English documentation for better international developer experience.
gridOptions?: DeepPartial<VxeTableGridOptions>; /** - * 显示搜索表单 + * Whether to show the search form + * 显示搜索表单 */ showSearchForm?: boolean;Also applies to: 61-64
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
110-140: Clean up commented code.The toolbar configuration is implemented correctly, but there are unnecessary commented-out code blocks that should be removed for better maintainability.
- // if (gridOptions.value?.toolbarConfig?.search) { - // } - // 强制使用固定的toolbar配置，不允许用户自定义 - // 减少配置的复杂度，以及后续维护的成本 toolbarConfig.slots = {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/src/components/common-ui/vben-vxe-table.md(3 hunks)packages/effects/plugins/src/vxe-table/api.ts(3 hunks)packages/effects/plugins/src/vxe-table/types.ts(4 hunks)packages/effects/plugins/src/vxe-table/use-vxe-grid.vue(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/src/components/common-ui/vben-vxe-table.md
🔇 Additional comments (6)
packages/effects/plugins/src/vxe-table/types.ts (1)
27-30: LGTM! Good choice making toolbarConfig optional.
The interface is well-defined and making toolbarConfig optional maintains backward compatibility with existing implementations.
packages/effects/plugins/src/vxe-table/api.ts (2)
24-24: LGTM! Good default value choice.
Setting showSearchForm to true by default maintains backward compatibility with existing implementations.
113-121: Verify if grid recalculation is needed after toggling.
The implementation is correct, but the commented-out recalculation code suggests potential layout issues. Consider testing with various scenarios to verify if grid recalculation is necessary.
✅ Verification successful
Let me search for more context about the grid component and visibility changes.
Let me search for one more thing to understand the grid's layout behavior.
Based on the gathered evidence, I can now provide the final response:
Grid recalculation after toggling search form can be safely removed
The codebase analysis shows that:
- The search form's visibility is controlled by a simple v-show directive in the template
- No other implementations require grid recalculation after visibility changes
- The form layout is handled independently through Vue's reactive system
The commented-out recalculation code appears to be unnecessary overhead and can be safely removed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations that might require recalculation after visibility changes
rg -A 3 'recalculate|updateLayout'
Length of output: 308
Script:
#!/bin/bash
# Search for grid-related visibility changes and layout updates
rg -A 5 'showSearchForm|show.*Form.*grid'
# Search for grid component implementation
ast-grep --pattern 'class $_ {
$$$
grid
$$$
}'
Length of output: 3263
Script:
#!/bin/bash
# Search for grid layout-related code and recalculation patterns
ast-grep --pattern 'grid.$$$recalculate'
# Look for form-related layout updates
rg -A 3 'form.*layout|layout.*form'
Length of output: 496
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (3)
4-7: LGTM! Good type imports.
The added type imports are appropriate and used correctly throughout the code.
337-341: LGTM! Good use of v-show directive.
Using v-show instead of v-if is a good choice for frequently toggled elements as it's more performant. The class handling is also implemented correctly.
196-203: 🛠️ Refactor suggestion
Improve type safety in event handling.
The event handling is implemented correctly, but the type assertion could be improved for better type safety.
- (
- gridEvents.value?.toolbarToolClick as VxeGridListeners['toolbarToolClick']
- )?.(event);
+ const handler = gridEvents.value?.toolbarToolClick;
+ if (handler && typeof handler === 'function') {
+ (handler as VxeGridListeners['toolbarToolClick'])(event);
+ }Likely invalid or redundant comment.
改成了showSearchForm，这个应该用的少，基本上也只是将搜索表单设为默认不显示时可能会用到。 |
Description
为表格添加控制搜索表单区域是否可见的方法，并添加一个名为search的工具按钮（可在toolbarConfig中配置是否可以显示）
Type of change
Please delete options that are not relevant.
pnpm-lock.yamlunless you introduce a new test example.Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit
Release Notes
New Features
Localization
Improvements