-
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
571fff7
to
0ac1f72
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 |
No description provided.