This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Implement the composite search bar for the mobile search header #1875
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1875 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. |
openverse-bot
added
the
🚦 status: awaiting triage
|
Size Change: +3.5 kB (0%) Total Size: 819 kB
ℹ️ View Unchanged
|
obulat
added
🟧 priority: high
obulat
force-pushed
the
composite_search_bar
branch
2 times, most recently
from
e76d32f to
d8a479f
Compare
zackkrida
reviewed
Oct 4, 2022
zackkrida
reviewed
Oct 4, 2022
zackkrida
reviewed
Oct 4, 2022
zackkrida
reviewed
Oct 4, 2022
zackkrida
suggested changes
Oct 4, 2022
There was a problem hiding this comment.
Very close to lgtm. Two things I noticed (one we can wait for @panchovm) on:
- The Openverse logo and the back button have different alignments so when you click on the input the text moves to the right slightly.
- The 'clear' button appears and is active even if there is no text in the input.
obulat
force-pushed
the
composite_search_bar
branch
2 times, most recently
from
abaf8f3 to
135000e
Compare
zackkrida
suggested changes
Oct 6, 2022
There was a problem hiding this comment.
- The changes to the logo/back button jumping look nice 👍 the logo is larger than advised by the mockups but when I test making it smaller locally it looks quite odd. We can review this with @panchovm later in the month.
- The 'clear' button still appears when there is no search term present. I believe this is fixed in Update desktop searchbar colors #1880 so as long as that continues I'm okay with this
- A small thing: the back button has no interaction or focus styles. Can this be updated with the universal styles from the button audit you completed too?
obulat
force-pushed
the
composite_search_bar
branch
from
ca08d36 to
5284e63
Compare
Replace internal border with an external ring on searchbar Show the Clear button only if there is a search term Darken the input text on hover
obulat
force-pushed
the
composite_search_bar
branch
from
350cb69 to
edc03c6
Compare
|
Changes since the last review:
|
dhruvkb
approved these changes
Oct 10, 2022
zackkrida
approved these changes
Oct 10, 2022
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1876 by @obulat
Description
This PR implements the new VHeaderMobile component that is used on mobile widths for the search result and single result pages.
It does not use the
VSearchBarorVInputFieldcomponents, copying some code from them instead. Re-using the existing components is difficult because of the different border treatments (in the current component, everything is inside an outer border, and in the existing VSearchBar, some components are outside of the border), and nesting several components and several levels of templaterefs is difficult to follow. We could extract the common functionality in the future, if that makes the code simpler.Testing Instructions
Go to 'http://localhost:8443/search/?q=cat&ff_new_header=on' on a narrow screen. The search bar should behave correctly. You should be able to update the search term, clear it, open (a stub of) the
VContentSettingsModal.You can also check out the VHeaderMobile story: https://wordpress.github.io/openverse-frontend/_preview/1875/?path=/story/components-vheader-vheadermobile-vheadermobile--default-story
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin