fix: table internal search collspaed configuration does not take effect #4585
Conversation
|
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (7)
packages/@core/ui-kit/form-ui/src/use-vben-form.ts (2)
29-29: LGTM with a suggestion for improvementThe change to include both
propsandattrsin the state is good for consistency. However, consider the order of spreading to ensure intended overriding behavior.Consider changing the order of spreading to prioritize
propsoverattrs:-api.setState({ ...props, ...attrs }); +api.setState({ ...attrs, ...props });This ensures that
propsvalues take precedence overattrsif there are any conflicting keys.
43-43: Approved, with a suggestion for robustnessUpdating the state with the entire
optionsobject is consistent with watching the whole object. This ensures all changes in options are reflected in the state.However, to prevent accidentally overwriting existing state that's not part of the options, consider merging the new options with the existing state:
-api.setState(options); +api.setState(prevState => ({ ...prevState, ...options }));This approach preserves any additional state that might have been set elsewhere while still updating with the new options.
packages/@core/ui-kit/form-ui/src/vben-use-form.vue (1)
Line range hint
60-65: LGTM: FormActions implementation with minor suggestionThe
FormActionscomponent is correctly implemented with conditional rendering and proper bindings for the collapsed state. This change addresses the core issue of the collapse functionality not working as intended.Consider using the shorthand
v-modelsyntax for better readability:- :model-value="state.collapsed" - @update:model-value="handleUpdateCollapsed" + v-model="state.collapsed"This change would simplify the code while maintaining the same functionality.
playground/src/views/examples/vxe-table/form.vue (3)
6-6: LGTM! Consider removing unused imports.The Button import is correctly added and used in the template. However, the
messageimport is not used in the visible code. If it's not used elsewhere in the component, consider removing it to keep the imports clean.
97-106: Function looks good, but there's a typo in the name.The
toggleFormCollspaefunction correctly implements the form collapse functionality, addressing the issue described in #4584. It resets the form and toggles the visibility of the collapse button as intended.However, there's a typo in the function name:
-function toggleFormCollspae() { +function toggleFormCollapse() {Also, ensure to update any references to this function name in the template or elsewhere in the code.
111-117: LGTM! Consider internationalization for button text.The addition of the toolbar with a button to toggle form collapse is well-implemented and aligns with the PR objectives. The button is correctly placed and linked to the
toggleFormCollspaefunction.However, consider the following suggestions:
- Update the function name in the template to match the corrected name (after fixing the typo):
-<Button type="primary" @click="toggleFormCollspae"> +<Button type="primary" @click="toggleFormCollapse">
- Consider using an internationalization approach for the button text to support multiple languages if required:
-切换表单折叠按钮 +{{ t('toggleFormCollapseButton') }}Replace with your preferred i18n method if different.
packages/effects/plugins/src/vxe-table/use-vxe-grid.ts (1)
11-11: Use camelCase for variable namingConsider renaming
IS_REACTIVEtoisReactiveOptionto adhere to JavaScript naming conventions. Uppercase names are typically reserved for constants, while camelCase is used for variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- packages/@core/ui-kit/form-ui/src/components/form-actions.vue (1 hunks)
- packages/@core/ui-kit/form-ui/src/use-vben-form.ts (2 hunks)
- packages/@core/ui-kit/form-ui/src/vben-use-form.vue (1 hunks)
- packages/effects/plugins/src/vxe-table/use-vxe-grid.ts (3 hunks)
- packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1 hunks)
- playground/src/views/examples/vxe-table/form.vue (2 hunks)
🧰 Additional context used
🔇 Additional comments (13)
packages/@core/ui-kit/form-ui/src/use-vben-form.ts (2)
Line range hint
1-53: Summary: Changes align with PR objectives and improve form handlingThe modifications in this file enhance the reactivity and state management of the form component, which aligns well with the PR objectives of fixing issues with form configuration. The changes to watch the entire
optionsobject and update the state accordingly should address the problems withformOptionsnot taking effect as mentioned in the linked issue #4584.Key improvements:
- More comprehensive state management by including both
propsandattrs.- Enhanced reactivity by watching the entire
optionsobject.- Consistent state updates with the full
optionsobject.These changes should help resolve the issue where
showCollapseButtonandcollapsedproperties were not functioning as intended. However, to fully verify the fix:#!/bin/bash # Description: Verify the implementation of showCollapseButton and collapsed properties # Search for usage of showCollapseButton and collapsed in formOptions rg --type typescript -e 'formOptions.*showCollapseButton' -e 'formOptions.*collapsed' # Search for the implementation of collapse functionality ast-grep --lang typescript --pattern $'function $_($_, $_) { $$$ showCollapseButton $$$ collapsed $$$ }'This script will help identify where and how these properties are used, allowing us to confirm that the changes in this file properly propagate to the collapse functionality implementation.
41-41: Approved, but consider performance implicationsWatching the entire
optionsobject provides more comprehensive reactivity. This change allows the component to respond to changes in any part of the options, which is generally good for maintaining consistency.However, this might lead to unnecessary re-renders if properties that don't affect the form are changed. To verify the impact, we can check how often the
optionsobject is modified:If the results show frequent modifications to properties that don't affect the form, consider using a more granular watch or implementing shouldUpdate logic to optimize performance.
packages/@core/ui-kit/form-ui/src/vben-use-form.vue (4)
Line range hint
30-32: LGTM: Addition ofhandleUpdateCollapsedfunctionThe new
handleUpdateCollapsedfunction correctly updates the collapsed state of the form using theformApi.setStatemethod. This addition aligns with the PR objective of fixing the collapse functionality issue.
42-42: LGTM: Addition of collapsed bindingThe new binding
:collapsed="state.collapsed"correctly connects theFormcomponent'scollapsedprop to thestate.collapsedvalue. This change ensures that the form's collapsed state is always in sync and reactive, which addresses the core issue described in the PR objectives.
Line range hint
1-70: Summary: Effective implementation of collapse functionalityThe changes in this file successfully address the issue described in the PR objectives. The implementation now provides a complete cycle for managing the form's collapsed state:
- The
handleUpdateCollapsedfunction allows updating the state.- The
Formcomponent is bound to the collapsed state.- The
FormActionscomponent provides user interaction to toggle the state.These changes should resolve the reported bug where the "Expand" button was not displayed and had no effect when clicked.
To ensure the changes are comprehensive, let's check if there are any other files that might need similar updates:
#!/bin/bash # Search for other files that might use similar form collapse functionality rg -l 'showCollapseButton|collapsed.*formOptions'This will help us verify if the fix has been applied consistently across the codebase.
Line range hint
3-3: Query: Removal of watch functionThe watch function for
form.valueshas been commented out. Can you provide more context on why this change was made? Removing this watch might affect how the component reacts to form value changes.To understand the impact, let's check if
handleValuesChangeis used elsewhere in the codebase:Also applies to: 5-5, 7-7
✅ Verification successful
**Verification Successful: No unintended layout changes detected.** All padding adjustments in `use-vxe-grid.vue` are isolated and consistent with existing patterns in the codebase. </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Check for any unintended layout changes in other components using similar classes # Test: Search for other components using pb-6 class that might need similar updates rg --type vue 'class="[^"]*pb-6' # Test: Verify the consistency of bottom padding across similar form containers rg --type vue 'class="[^"]*relative rounded py-3 pb-[46]'Length of output: 150
Script:
#!/bin/bash # Description: Check for any unintended layout changes in other components using similar classes # Test: Search for other components using pb-6 class that might need similar updates rg 'class="[^"]*pb-6' --type-add 'vue:*.vue' --type vue # Test: Verify the consistency of bottom padding across similar form containers rg 'class="[^"]*relative rounded py-3 pb-\[46\]' --type-add 'vue:*.vue' --type vueLength of output: 227
Script:
#!/bin/bash # Description: Check for any unintended layout changes in other components using similar padding classes # Test: Search for other components using pb-6 class that might need similar updates rg 'class="[^"]*pb-6' --type-add 'vue:*.vue' --type vue # Test: Search for components using pb-4 class to ensure consistency rg 'class="[^"]*pb-4' --type-add 'vue:*.vue' --type vueLength of output: 453
packages/effects/plugins/src/vxe-table/use-vxe-grid.ts (2)
3-3: Appropriate import of 'watch' from 'vue'The addition of
watchto the import statement is necessary for implementing reactivity support in the code.
23-23: Synchronizing API state with props and attrsMerging
propsandattrsintoapi.setStateensures that the API state reflects the current component properties and attributes. This practice keeps the API state consistent with the component's external interface.
Description
fixed #4584
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