-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Restored search hotkey for desktop. (alternative) #2442
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
adamzap
left a comment
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.
Great work here, @sarahboyce! This is the kind of solution I was hoping for.
We should confirm that this design change on wide viewports is intended and ideal:
Before:
After:
I think it's a nice improvement!
I'll take a closer look at the Playwright integration.
| </nav> | ||
| <div class="header-tools"> | ||
| {% search_form %} | ||
| {% include "includes/toggle_theme.html" %} |
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 suppose we could remove this include now, but it's probably out of scope here.
It wasn't intentional, I also think I prefer it to the current state. I know ülgen pointed out in the slack group that prior to the previous navbar change the navbar was centered on wide viewpoints. So perhaps we can chose what we want and make some adjustments 👍 |
|
@sarahboyce We discussed this PR in the WG meeting today. Could you move the Playwright parts of this to another PR so that we can iterate on front-end testing separately? |
2bc24dd to
aac8e4d
Compare
Moved to #2444 |
adamzap
left a comment
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.
👍 Looks great!
SaptakS
left a comment
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 like the order CSS trick to arrange the items in the navbar. Looks good to me. I did notice a separate accessibility issue, which is the keyboard seems to tab through the entire menu in the mobile navigation even when the menu is not open, but that should be a separate PR.
Fixes #2349
I also added playwright to have a way to test JavaScript (might be controversial and can remove if wanted)
This is an alternative to #2355. Here, only one search bar exists in the page