-
Notifications
You must be signed in to change notification settings - Fork 993
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
Fixes #35601 - Search in navigation #9681
Conversation
MariaAga
commented
Apr 12, 2023
•
edited
Loading
edited
Issues: #30344 |
a18ee38
to
393f9bf
Compare
@MariaAga could you pls add also a screenshot of an inactive search bar (default state)?
|
Here is a link to a demo which hopefully answer most questions: https://hmj7ff.csb.app/ (its a bit jumpy when navigating) |
@MariSvirik Thanks!
|
@MariSvirik ping |
@MariSvirik I also made a demo that doesnt use expendable search, but just a search input here: https://l9hdpm.csb.app/audits |
Talked to Maria S and we decided on this design: |
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.
Thanks, @MariaAga! Great addition. No more trying to remember where is $ITEM located :)
Few comments:
- Is there a way to trigger the nav search via keyboard? Looking at the tickets, there was an experimental key ` as an example.
- It would be nice if the first item from the suggestion list was auto-selected, so I don't need to use arrows for explicit selection.
- Some comments inline.
- Test failures seem to be related?
- Huge thanks for the explanations in the comments :)
webpack/assets/javascripts/react_app/components/Layout/NavigationSearch.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/Layout/NavigationSearch.js
Outdated
Show resolved
Hide resolved
Thanks!
added CTRL + SHIFT + F shortcut as ` was mentioned not being easily accessible in some keyboard layouts
I'm not sure if that what you meant, but now if a user starts typing in the search and clicks ENTER the page will redirect to the first result Added more specific selectors to failing tests |
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.
Thanks, @MariaAga! One inline question, but otherwise good to go :)
const searchInput = ( | ||
<SearchInput | ||
value={value} | ||
placeholder={__('Search and go')} |
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.
Could we change it to Ctrl + Shift + F to search
? I mean, how would one know what to use?
Not a big deal though, since that combination is used in a lot of editors...
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.
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.
Love it, thanks @MariaAga! Not sure though if we can merge it now or should we wait for branching...
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.
Thanks, @MariaAga, apparently with this one we can go further :) Promise me, you'll re-check we didn't break anything for most wanted plugins :D