-
Notifications
You must be signed in to change notification settings - Fork 4k
Revamp mobile navigation #3282
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
base: main
Are you sure you want to change the base?
Revamp mobile navigation #3282
Conversation
🦋 Changeset detectedLatest commit: ea170d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
0f4a839
to
af27aba
Compare
Summary of the deployments: Version 1
Version 2
Test content |
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
af27aba
to
de795ca
Compare
d66bc22
to
1e5218e
Compare
1e5218e
to
191dd67
Compare
0ac1f72
to
6faa5cf
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.
I think it can negatively impact perfs as it means the TOC will be rendered twice; we have spaces with thousands of pages, which can result in a big HTML chunk just for mobile
packages/gitbook/src/components/Header/mobile-menu/MobileMenuSheet.tsx
Outdated
Show resolved
Hide resolved
packages/gitbook/src/components/Header/HeaderMobileMenuButton.tsx
Outdated
Show resolved
Hide resolved
@SamyPesse Probably not the sexiest approach, but this way we can preserve the initial HTML (desktop TOC) and switch to the Sheet TOC for mobile, so no duplicate I’m not a big fan of relying on I still think it's a good improvement and it won't change anything on desktop |
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.
I tested it, the animation is not great and the backdrop is blinking and disappearing.
It's still need to be polished before being ready to be merged.
CleanShot.2025-06-12.at.21.48.57.mp4
// biome-ignore lint/correctness/useExhaustiveDependencies: Close the navigation when navigating to a page | ||
useEffect(() => { | ||
setOpen(false); | ||
}, [pathname]); |
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.
Why do we have this logic in the MobileMenuButton and not elsewhere? A Provider or something like that?
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.
You will re-render the button when the pathname change for nothing.
// If the screen is mobile, we use the mobile menu sheet to display the table of contents. | ||
if (isMobile) { | ||
return <MobileMenuSheet>{children}</MobileMenuSheet>; | ||
} |
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.
Why do we need that. I don't understand why we don't keep the same element and we just switch its position to absolute or something like that? In the app, in mobile I use another layout but we could keep the same component like in desktop actually (I think I will refactor it at some point). I think we could do something smarter here.
No description provided.