-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
refactor: move color mode toggle in navbar on mobiles #6976
base: main
Are you sure you want to change the base?
Conversation
a462588
to
516a1b7
Compare
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-6976--docusaurus-2.netlify.app/ |
Size Change: -71 B (0%) Total Size: 805 kB
ℹ️ View Unchanged
|
key="toggle" | ||
className={isMobile ? 'margin-left--sm' : undefined} | ||
/>, | ||
!hasExplicitSearchItem || isMobile ? <SearchBar /> : null, |
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 needs a key
if (isMobile) { | ||
[rightMostItems[0], rightMostItems[1]] = [ | ||
rightMostItems[1]!, | ||
rightMostItems[0]!, | ||
]; | ||
} |
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.
Since the search button just opens a modal, can we not do the swap and always put the search as the rightmost item?
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 did this intentionally because some custom search input can be expanded by click, so it's probably better to try to freeze the color mode toggle position. I may be wrong though, and it's a completely normal option, just compare:
Option 1 (default) | Option 2 (with swapping items on mobiles) |
---|---|
![]() |
![]() |
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 see. If there are custom search inputs that don't open a modal then it makes sense
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.
Not so fan of this change in terms of UX and SSR/CSR behavior
This swap of theme/search leads to an issue.
On a mobile with slow connexion:
- you see first theme/search
- it then flips to search/theme
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.
The mobile hydration layout change should rather be avoided
This is a quite significant UX change, wonder if we shouldn't poll our community about it?
Some sites might be annoyed by this change, particularly those that want to keep a larger theme toggle instead of a theme switch button (for example https://supabase.com/docs ?)
This is not so easy for users to revert to the old behavior too if they want that.
Wonder if we shouldn't have a broader redesign of the mobile theme design, including the API
For example maybe we should be able to set a smaller label for mobile? or having another array of navbar items dedicated for mobile?
if (isMobile) { | ||
[rightMostItems[0], rightMostItems[1]] = [ | ||
rightMostItems[1]!, | ||
rightMostItems[0]!, | ||
]; | ||
} |
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.
Not so fan of this change in terms of UX and SSR/CSR behavior
This swap of theme/search leads to an issue.
On a mobile with slow connexion:
- you see first theme/search
- it then flips to search/theme
Yeah, I don't really like the current implementation either, so guess additional refactoring is needed here. And I agree about doing a community poll about the current change. |
Motivation
Suggest finally moving the color mode toggle to the navbar itself, instead of the sidebar on mobiles. Since now it represents a compact button, it fits nicely into the navbar area. It's also more consistent with desktop design, and just it's clearer to the user that color scheme of a site can be changed at all.
However there's another, more important reason: the mobile sidebar didn't have much space for long site title, and I saw that some site title couldn't always be fully displayed. Just to give you an example:
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Related PRs