Repository navigation
fix(@vben/common-ui): resolve #6698 - #6700
Conversation
|
WalkthroughContent and layout updates to the Page component: content height calculation now subtracts header and footer heights plus offset; container classes switched to a vertical flex layout and inner content set to flex-1; footer removed from absolute positioning and rendered in normal document flow. No public API changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Page
participant Header
participant Footer
participant Content
User->>Page: Load Page
Page->>Header: measure headerHeight
Page->>Footer: measure footerHeight
Page->>Content: compute height = var(--layout-content-height) - headerHeight - footerHeight - heightOffset
Note over Content: Content uses min-h-0 and flex-1 to fill remaining vertical space
Footer-->>Page: rendered in normal flow (not absolute)
Estimated code review effortπ― 2 (Simple) | β±οΈ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
π Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro π‘ Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. π Files selected for processing (1)
π§ Files skipped from review as they are similar to previous changes (1)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
β¨ Finishing Touchesπ§ͺ Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. πͺ§ TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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
π§Ή Nitpick comments (2)
packages/effects/common-ui/src/components/page/page.vue (2)
25-33: Guard against negative heights when offsets exceed container.
Wrap the calc with CSS max() to avoid collapsing to a negative height in edge cases (small viewports, large header/footer, big heightOffset).- height: `calc(var(${CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT}) - ${headerHeight.value}px - ${footerHeight.value}px - ${typeof heightOffset === 'number' ? `${heightOffset}px` : heightOffset})`, + height: `max(0px, calc(var(${CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT}) - ${headerHeight.value}px - ${footerHeight.value}px - ${typeof heightOffset === 'number' ? `${heightOffset}px` : heightOffset}))`,
95-100: Remove no-op positioning classes and fix alignment utility.
After dropping absolute positioning, bottom-0/left-0/right-0 do nothing. Also verify βalign-centerβ (Tailwind uses items-center).- 'bg-card align-center bottom-0 left-0 right-0 flex px-6 py-4', + 'bg-card flex items-center px-6 py-4',Optionally add a top border for parity with the header:
- 'bg-card flex items-center px-6 py-4', + 'bg-card flex items-center border-t border-border px-6 py-4',
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π‘ Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
π Files selected for processing (1)
packages/effects/common-ui/src/components/page/page.vue(2 hunks)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Check (windows-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
π Additional comments (1)
packages/effects/common-ui/src/components/page/page.vue (1)
28-28: LGTM: subtracting footerHeight fixes overflow with in-flow footer.
This aligns content height with header+footer presence.
|
deal with another situation |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and canβt be posted inline due to platform limitations.
β οΈ Outside diff range comments (1)
packages/effects/common-ui/src/components/page/page.vue (1)
25-33: Make header/footer height updates reactive
contentStyle (lines 25β33) uses headerHeight.value and footerHeight.value, but calcContentHeight() is only called once onMounted (line 47), so element or viewport resizes wonβt recalc layout, causing stale scroll areas. Add a ResizeObserver on headerRef/footerRef (and optional window.resize listener) to update those refs on size changes, and disconnect/cleanup in onUnmounted.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π‘ Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
π Files selected for processing (1)
packages/effects/common-ui/src/components/page/page.vue(4 hunks)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test (windows-latest)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
π Additional comments (1)
packages/effects/common-ui/src/components/page/page.vue (1)
53-53: Verify Flex-Column Footer Impact
Flex-column switch can break pages expecting a fixed/absolute footer. I ran a search for<Page>consumers using a footer slot but found noneβplease manually verify that no pages rely on the previous absolute-positioned footer. If any do, preserve legacy behavior by adding afooterFixed?: booleanprop.
There was a problem hiding this comment.
Actionable comments posted: 0
β»οΈ Duplicate comments (1)
packages/effects/common-ui/src/components/page/page.vue (1)
89-89: LGTM and thanks for adopting min-h-0.Replacing h-full with min-h-0 on the flex child prevents double scroll/overflow traps.
π§Ή Nitpick comments (1)
packages/effects/common-ui/src/components/page/page.vue (1)
28-30: Rely on flex; drop JS height calc to avoid stale measurements and overflow flicker.With the new flex column layout and footer in normal flow, explicit height math is unnecessary and brittle on resize/content changes. Let flex + min-h-0 handle it.
Apply:
- height: `calc(var(${CSS_VARIABLE_LAYOUT_CONTENT_HEIGHT}) - ${headerHeight.value}px - ${footerHeight.value}px - ${typeof heightOffset === 'number' ? `${heightOffset}px` : heightOffset})`, - overflowY: shouldAutoHeight.value ? 'auto' : 'unset', + overflowY: 'auto',If keeping JS sizing, consider observing header/footer with ResizeObserver to react to dynamic height changes.
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π‘ Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
π Files selected for processing (1)
packages/effects/common-ui/src/components/page/page.vue(3 hunks)
β° Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
π Additional comments (2)
packages/effects/common-ui/src/components/page/page.vue (2)
53-53: LGTM: root switched to flex column.This aligns structure with the scrolling model change.
95-95: Remove invalid class; center with items/justify utilities.align-center isnβt a standard Uno/Tailwind class. Use items-center (and optionally justify-center) instead.
[ suggest_nitpick ]
[ duplicate_comment ]
Apply:- :class="cn('bg-card align-center flex px-6 py-4', footerClass)" + :class="cn('bg-card flex items-center px-6 py-4', footerClass)"
* fix(@vben/common-ui): resolve vbenjs#6698 * fix(@vben/common-ui): resolve vbenjs#6698 * fix(@vben/common-ui): resolve conversation * fix(@vben/common-ui): resolve vbenjs#6698
Description
before modification
after modification
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