-
Notifications
You must be signed in to change notification settings - Fork 112
Fix mobile menu for KeystarUI docs #1472
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
| {nextRootScript} | ||
| <style | ||
| dangerouslySetInnerHTML={{ | ||
| __html: /* css */ `html, body { height: 100%; }`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const pathname = usePathname(); | ||
|
|
||
| useEffect(() => { | ||
| addEventListener('popstate', closeSidebar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't working to close the menu
Current experience:
https://github.com/user-attachments/assets/befa47e4-bfa5-42ef-a220-c5466045c36b
New experience:
https://github.com/user-attachments/assets/7b6bc1f6-da71-4499-892f-2b6615787b04
| eslint: { ignoreDuringBuilds: true }, | ||
| typescript: { ignoreBuildErrors: true }, | ||
| serverExternalPackages: ['esbuild'], | ||
| experimental: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unrelated, but Next.js was complaining about this experimental flag being deprecated and to use serverExternalPackages instead.
84c5574 to
2bb0924
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes the mobile menu scrolling issue in the KeystarUI documentation by implementing a proper flexbox-based layout with percentage-based heights instead of viewport units.
Key changes:
- Updated Next.js configuration to use the stable
serverExternalPackagesAPI (migrated from experimental) - Implemented proper flexbox hierarchy with
minHeight={0}to enable scrolling in the mobile sidebar - Changed from viewport-based heights (
100vh,100vw) to percentage-based (100%) with explicit html/body height styling
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
design-system/docs/next.config.mjs |
Migrated serverComponentsExternalPackages from experimental to stable serverExternalPackages API for Next.js 15 compatibility; removed commented-out configuration |
design-system/docs/components/sidebar/sidebar.tsx |
Restructured sidebar layout with proper flexbox container, fixed CSS selector for open state, changed navigation close behavior to trigger on pathname changes, and added minHeight={0} for scrollable content |
design-system/docs/components/layout.tsx |
Changed outer container height from 100vh to 100% to support percentage-based height chain |
design-system/docs/components/content/index.tsx |
Converted content wrapper from Box to Flex with column direction and added flex property to enable proper flex layout |
design-system/docs/app/(site)/layout.tsx |
Added global styles to set html and body height to 100% to establish proper height inheritance chain |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


This fixes the mobile menu for the docs, which was not allowing for scrolling.