fix: improve the layout of tables offline on the mobile #4573
Conversation
|
WalkthroughThe pull request introduces several changes across multiple components, including modifications to the Changes
Assessment against linked issues
Possibly related PRs
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/effects/plugins/src/vxe-table/theme.css (3)
Line range hint
69-78: Improved pager layout structure. Consider adding responsive behavior.The changes to the
.vxe-pagerclass enhance the layout structure by using flexbox, which improves alignment and potentially responsiveness. The.vxe-pager--sizesclass pushing to the left side is a good touch for layout control.Consider adding responsive behavior to the
.vxe-pager--sizesmargin. For example:.vxe-pager--sizes { margin-right: auto; @media (max-width: 768px) { margin-right: 0; margin-bottom: 10px; } }This would stack the elements vertically on smaller screens, potentially improving the mobile layout further.
80-82: Responsive justification added. Consider using Tailwind throughout for consistency.The addition of the
.vxe-pager--wrapperclass with responsive justification is a good improvement for mobile layouts. It centers content on smaller screens and aligns to the end on medium screens and up.For consistency, consider using Tailwind utilities throughout the file where applicable. For example, you could refactor the
.vxe-pager--sizesrule to use Tailwind:.vxe-pager { &--wrapper { @apply flex items-center justify-center md:justify-end; } &--sizes { @apply mr-auto; } }This would maintain a consistent styling approach across the component.
Line range hint
1-82: Overall improvements align with PR objectives. Consider standardizing CSS approach.The changes in this file successfully address the layout issues for mobile devices as outlined in the PR objectives. The use of flexbox and responsive justification in the pager component will significantly improve the mobile experience.
To further enhance the maintainability and consistency of the codebase:
Consider standardizing the CSS approach throughout the file. You're currently mixing traditional CSS with Tailwind utilities. Choosing one approach (preferably Tailwind, given its use in the new code) could improve readability and maintainability.
Review the commented-out CSS variables at the top of the file. If they're no longer needed, consider removing them to keep the file clean and up-to-date.
Ensure that these changes are thoroughly tested on various mobile devices and screen sizes to confirm that the layout improvements work as intended across different contexts.
These suggestions will help maintain a consistent and efficient codebase as the project evolves.
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (1)
165-166: Consider includingcollapseTriggerResizeinfinalFormOptionsAdding
collapseTriggerResizeseparately might be inconsistent. Consider including it withinfinalFormOptionsfor consistency.Apply this diff to include
collapseTriggerResizeinfinalFormOptions:const finalFormOptions: VbenFormProps = mergeWithArrayOverride( {}, formOptions.value, defaultFormProps, + { + collapseTriggerResize: !!defaultFormProps.showCollapseButton, + }, ); - return { - ...finalFormOptions, - collapseTriggerResize: !!finalFormOptions.showCollapseButton, - }; + return finalFormOptions;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/@core/ui-kit/form-ui/src/components/form-actions.vue (1 hunks)
- packages/effects/plugins/src/vxe-table/theme.css (1 hunks)
- packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (6 hunks)
- playground/src/locales/langs/zh-CN.json (1 hunks)
🔇 Additional comments (7)
packages/@core/ui-kit/form-ui/src/components/form-actions.vue (1)
Line range hint
33-44: Reconsider the changes to form layout logicThe modifications to the
isQueryFormcomputed property andqueryFormStyleraise concerns:
Commenting out
isQueryForm(lines 33-35) removes the logic for determining when to show the collapse button. This doesn't align with the PR objective of improving mobile layout and the linked issue's discussion about hiding the query button under certain conditions.The change in
queryFormStyle(line 38) now relies on the absence ofactionWrapperClassrather thanisQueryForm. This may not accurately represent whether it's a query form and could lead to unexpected layout issues.Consider the following improvements:
- Instead of commenting out
isQueryForm, modify it to incorporate the conditions discussed in the linked issue (e.g., number of form elements).- Update
queryFormStyleto use bothisQueryFormandactionWrapperClassfor more precise control over the layout.- Ensure these changes improve the layout on mobile devices as per the PR objective.
Example refactor:
const isQueryForm = computed(() => { const { showCollapseButton, formItems } = unref(rootProps); return showCollapseButton && formItems.length > 2; // Adjust the condition as needed }); const queryFormStyle = computed(() => { if (unref(isQueryForm) && !unref(rootProps).actionWrapperClass) { return { 'grid-column': `-2 / -1`, marginLeft: 'auto', }; } return {}; });To ensure these changes align with the PR objectives, please run the following verification:
This will help us understand the broader context and ensure our changes don't introduce regressions.
playground/src/locales/langs/zh-CN.json (1)
95-95: Approved, but clarification needed on relevance to PR objectivesThe change from "开启搜索表单" (Open Search Form) to "搜索表单" (Search Form) is a valid simplification of the UI text. However, it's not clear how this change relates to the PR objectives of improving table layout on mobile devices or the linked issue (#4570) about hiding query buttons and table layout problems.
Could you please clarify how this localization change contributes to the stated objectives of the PR? Are there other changes in different files that work in conjunction with this text modification to address the mobile layout issues?
To verify if there are related changes, let's check for modifications in other localization files and any components that might use this text:
packages/effects/plugins/src/vxe-table/use-vxe-grid.vue (5)
22-22: Import ofusePreferencesis appropriateThe addition of
usePreferencesimport aligns with its usage in the component.
52-53: DestructuringisMobilefromusePreferencesCorrectly obtaining
isMobilefor responsive design adjustments.
158-163: Review the order of merging form optionsIn
mergeWithArrayOverride, the default form properties are merged afterformOptions.value, which may override user-provided values. Ensure this is intentional and that default properties should take precedence overformOptions.value.
259-259: Validate Tailwind CSS classes for consistencyThe class string includes responsive classes like
md:bottom-2andmd:h-3. Ensure these classes achieve the desired styling across different screen sizes.
119-119: Ensure reactivity ofisMobile.valuein computed propertiesVerify that
isMobile.valueis reactive so thatlayoutsinpagerConfigupdate accordingly when the device type changes.Run the following script to check if
isMobileis a reactive property:
Description
fixed #4570
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
New Features
Bug Fixes
Style
Refactor