-
Notifications
You must be signed in to change notification settings - Fork 211
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
Update recent searches to match updated mockups #3796
Conversation
Full-stack documentation: https://docs.openverse.org/_preview/3796 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. |
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 only noticed one improvement on the popover version, but on full-screen one there are spacing differences.
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.
LGTM and works as expected locally!
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.
Regarding components, I noticed that text style in Popover item is incorrect. It should be "label regular."
In that same line, the disclaimer text is based on label regular but with line-height of 1.4 to not look tight on mobile
Small breakpoints (xs, sm, md
)
This view is what needs more spacing improvement. The alignment of texts and icons is different and disclaimer aligned at the bottom is key to keep the item list clean when increases. Here is a comparison of what's on the design and what I see in dev.
Mockup | Dev |
---|---|
I added spec notes for the small breakpoints view in one of the XS mockups. |
Oh I see. Then, placing the disclaimer below items is completely fine. I will update the mockups for documentation purposes. |
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @obulat Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @dhruvkb, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
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 changes look awesome!
The one change that should be added is the clear-single
action for mobile:
diff --git a/frontend/src/components/VHeader/VHeaderMobile/VHeaderMobile.vue b/frontend/src/components/VHeader/VHeaderMobile/VHeaderMobile.vue
--- a/frontend/src/components/VHeader/VHeaderMobile/VHeaderMobile.vue (revision 89eec45394c45d5ce7ed95fc3f05269aa211a552)
+++ b/frontend/src/components/VHeader/VHeaderMobile/VHeaderMobile.vue (date 1709726975300)
@@ -104,6 +104,7 @@
class="mt-4"
@select="handleSelect"
@clear="handleClear"
+ @clear-single="handleClearSingle"
/>
</ClientOnly>
</VInputModal>
@@ -278,6 +279,13 @@
ensureFocus(searchInputRef.value)
}
}
+ /* Clear a specific recent search from the store. */
+ const handleClearSingle = (idx: number) => {
+ if (searchInputRef.value) {
+ ensureFocus(searchInputRef.value)
+ }
+ searchStore.clearRecentSearch(idx)
+ }
const showRecentSearches = computed(
() => isRecentSearchesModalOpen.value && entries.value.length > 0
@@ -330,6 +338,7 @@
handleKeydown,
handleSelect,
handleClear,
+ handleClearSingle,
}
},
})
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.
It looks great ✨
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.
LGTM!
Fixes
Fixes #3194 by @fcoveram
Description
This PR
There is a bit of a problem where the recent searches cannot individually be cleared using the keyboard because the keyboard navigation inside the popup does not cover them, but I feel that is acceptable because all recent searches can still be cleared using the keyboard and there is no navigational order that would feel natural for iterating over the popup suggestions as well as their × icons.
Testing Instructions
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin