-
Notifications
You must be signed in to change notification settings - Fork 447
Fix search dropdown behaviour #1512
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
|
🚀 Preview deployment available at: https://3aedfc70.rdoc-6cd.pages.dev (commit: d461adf) |
823a0b2 to
fdddc9f
Compare
| // Hide search results when clicking outside the search area | ||
| document.addEventListener('click', (e) => { | ||
| if (!e.target.closest('.navbar-search-desktop')) { | ||
| result.setAttribute('aria-expanded', 'false'); |
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.
Up and down arrow keys still moves in the hidden search navigation.
I think search.setNavigationActive(false) search.setNavigationActive(true) are needed where changing aria-expanded attribute.
Adding search.hide() and search.show() that does both might be a good idea.
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.
Ah good catch. Updated 👍
| # RDoc version you are using | ||
|
|
||
| VERSION = '7.0.1' | ||
| VERSION = '7.0.2' |
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.
Maybe wrong commit pushed?
I'm fine cutting 7.0.2
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.
No I just thought if we could merge the PR we can ship it immediately in 7.0.2 😛
This would still work tho. I'd love to cut a new release right after this is merged.
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 👍
7ad4bbb to
5d2565b
Compare
After this change: | Scenario | Behavior | |----------|----------| | Type search, click result | Navigate without `?q=`, clean URL without dropdown | | Type search, press Enter | Navigate without `?q=`, clean URL without dropdown | | Land on URL with `?q=` param | Show dropdown with pre-filled query | | Click outside dropdown | Hide dropdown, keep input value | | Focus input with existing query | Show dropdown again |
5d2565b to
d461adf
Compare
tompng
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.
👍
After this change:
?q=, clean URL without dropdown?q=, clean URL without dropdown?q=param