-
Notifications
You must be signed in to change notification settings - Fork 63
Close the recent searches on Shift+Tab #1906
Conversation
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1906 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
Size Change: -3.74 kB (0%) Total Size: 818 kB
ℹ️ View Unchanged
|
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 is good, except a non-blocking nit.
@@ -211,6 +211,11 @@ export default defineComponent({ | |||
// If a recent search is selected, populate its value into the input. | |||
modelMedium.value = entries.value[selectedIdx.value] | |||
|
|||
// Hide the recent searches popover when the user presses shift+tab on the input. | |||
if (key === keycodes.Tab && event.shiftKey) { | |||
handleBlur() |
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'm a little bothered by the difference between this line and line 221 below.
I'm not sure if we should use isRecentVisible.value = false
to match the code in the next if
block or update that to use handleBlur()
instead.
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 haven't noticed it before, and I'm all in for consistency! I wonder if it's best to extract a hideRecent
function that would set isRecentVisible.value
to false
to make it easier to understand the code.
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've renamed handleBlur
to hideRecentSearches
, and handleFocus
to showRecentSearches
(they actually just set the isRecentVisible.value
to false
/true
), @dhruvkb. I feel like that makes it easier to understand the template code.
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.
Nice change!
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.
👍
* Close the recent searches on Shift+Tab * Rename function to clarify their meanings * Improve clarity by renaming/moving code
Fixes
Fixes #1886 by @zackkrida
Description
This is a follow up for 1889 to close the Recent searches popover when pressing Shift+Tab while inside the input (when you get to the Openverse logo).
Testing Instructions
On main, when you press Shift+Tab while inside the search input, you focus the Openverse logo, but the Recent searches popover stays open. Here, it should close.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin