fix: mixed menu layout in full content mode #4990
Conversation
|
WalkthroughThe changes in this pull request primarily focus on modifying the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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/@core/ui-kit/layout-ui/src/components/layout-sidebar.vueOops! Something went wrong! :( ESLint: 9.15.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/node_modules/@vben/eslint-config/dist/index.mjs' imported from /eslint.config.mjs 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: 0
🧹 Outside diff range and nitpick comments (1)
packages/@core/ui-kit/layout-ui/src/components/layout-sidebar.vue (1)
Line range hint
194-211: Consider improving width calculation maintainability.The width calculation logic is spread across multiple places and handles various states. Consider these improvements:
- Extract width calculation into a separate composable for better reusability
- Add JSDoc comments documenting the different width states
- Consider using TypeScript const assertions for width values
Example refactor:
/** * Composable for sidebar width calculations * @param props - Sidebar props * @param state - Sidebar state (collapse, extraVisible, etc.) * @returns Computed width styles */ function useSidebarWidth(props: Props, state: SidebarState) { return computed(() => { const width = props.width === 0 ? 0 : props.width + (props.isSidebarMixed && props.fixedExtra && state.extraVisible ? props.extraWidth : 0); const widthPx = `${width}px` as const; return { ...(width === 0 ? { overflow: 'hidden' as const } : {}), flex: `0 0 ${widthPx}`, marginLeft: props.show ? 0 : `-${widthPx}`, maxWidth: widthPx, minWidth: widthPx, width: widthPx, }; }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@core/ui-kit/layout-ui/src/components/layout-sidebar.vue(1 hunks)packages/@core/ui-kit/layout-ui/src/vben-layout.vue(1 hunks)
🔇 Additional comments (2)
packages/@core/ui-kit/layout-ui/src/components/layout-sidebar.vue (1)
194-197: LGTM! The width calculation fix addresses the layout issue.
The change correctly handles the case when width is 0, preventing layout disruption in full-screen mode.
Let's verify the fix works across different viewport sizes and collapse states:
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows that the width calculation change is part of a larger layout system that handles different sidebar modes (mixed, collapsed, etc.) consistently across the application.
The width calculation change is correct and consistent with the codebase design.
The search results confirm that:
- The
width === 0check is necessary for proper collapse behavior in the layout system - The width calculation is used in conjunction with
isSidebarMixedmode across multiple components - The change aligns with the existing layout logic in
vben-layout.vuewhich also handles special cases for width being 0 in mobile and collapsed states
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find related layout tests and viewport handling code
# Search for related layout tests
rg -l "layout.*test" --glob "*.{spec,test}.{ts,js,vue}"
# Search for responsive/viewport related code
ast-grep --pattern 'computed(() => {
$$$
width $$$
$$$
})'
Length of output: 124
Script:
#!/bin/bash
# Let's try a different approach to find layout-related tests and viewport handling
# Search for test files containing 'layout' or 'sidebar'
rg -l "layout|sidebar" --glob "*.{spec,test}.{ts,js,vue}"
# Search for width-related computations in Vue files
rg -A 5 "computed.*width" --glob "*.vue"
# Search for responsive or viewport-related code
rg -A 5 "(responsive|viewport|breakpoint)" --glob "*.{vue,ts}"
# Look for similar width calculations in other components
ast-grep --pattern 'width === 0 ? $$ : $$'
Length of output: 8354
Script:
#!/bin/bash
# Let's search for related layout sidebar components and width calculations
# Search for similar sidebar components
rg -A 10 "layout-sidebar" --glob "*.{vue,ts}"
# Look for width-related style calculations
rg -A 5 "width.*style" --glob "*.vue"
# Search for similar mixed sidebar logic
rg -A 5 "isSidebarMixed" --glob "*.{vue,ts}"
# Look for width-related tests or stories
rg -l "width.*test|sidebar.*test" --glob "*.{spec,test,stories}.{ts,js,vue}"
Length of output: 13871
packages/@core/ui-kit/layout-ui/src/vben-layout.vue (1)
195-195: LGTM! The sidebar visibility logic fix looks correct.
The addition of !props.sidebarHidden condition ensures proper sidebar hiding behavior, which should resolve the mixed menu layout issues in full content mode.
Let's verify the sidebar visibility behavior:
✅ Verification successful
Based on the search results, I can now verify the review comment. The code shows that:
-
The
showSidebarcomputed property is used to control sidebar visibility and is referenced in multiple places:- Template:
:show="showSidebar"for sidebar component - Used in
mainStylecomputed property for width calculations - Affects header wrapper styling
- Template:
-
The
isSideModecomputed property correctly checks for sidebar-based layouts:
const isSideMode = computed(() =>
currentLayout.value === 'mixed-nav' ||
currentLayout.value === 'sidebar-mixed-nav' ||
currentLayout.value === 'sidebar-nav'
);- The
sidebarEnablevalue is properly used in conjunction with layout mode:
const sidebarEnableState = computed(() => {
return !isHeaderNav.value && sidebarEnable.value;
});The sidebar visibility logic is correctly implemented.
The addition of !props.sidebarHidden condition to showSidebar computed property ensures proper sidebar hiding behavior in all layout modes. This change works in harmony with the existing layout mode checks and sidebar enable state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other sidebar visibility related logic that might need similar updates
# Search for sidebar visibility related code
rg -A 5 'sidebarHidden|showSidebar|sidebar.*visible' --type vue
# Search for potential layout mode checks
ast-grep --pattern 'computed(() => {
$$$
return $_ && $_ && !props.sidebarHidden
$$$
})'
Length of output: 190
Script:
#!/bin/bash
# Let's try a different approach to search for sidebar-related code
# Search for sidebar-related code without file type restriction
rg -A 5 'sidebarHidden|showSidebar|sidebar.*visible'
# Search for computed properties in .vue files
rg -A 5 'computed\(' --type-add 'vue:*.vue' --type vue
# Search for layout mode related code
rg -A 5 'isSideMode|sidebarEnable' --type-add 'vue:*.vue' --type vue
Length of output: 126392
Description
修复混合菜单布局在内容区域最大化时侧边菜单的样式问题。fixed #4989
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