Add search button to header #3483
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
precious-onyenaucheya-ons
force-pushed
the
feature/162-header-search-button
branch
from
5a42c21 to
7bf2beb
Compare
There was a problem hiding this comment.
I've got some alignment issues when I view this on my browser
Website:
Design:
- It looks like the icon in the button is too small and isn't centred.
- The input looks like perhaps it's not tall enough which makes the button next to the input look squashed.
- The button next to the input also looks like it has some sort of black border at the bottom which doesn't appear in the designs.
- The design also seems to show a blue border around the input whereas it looks like it currently has a black border. We might need some clarification about how this button should look when clicked as that doesn't seem to be included in the design. At the moment this looks very odd to me.
|
The search button doesn't seem to have any focus state at the moment, which doesn't match new menu button. It also doesn't have an underline like that button when it is open, which I know is not in the figma but I think was suggested after. We need this to be consistent with the way the menu button works. Also has this been tested alongside the menu button? I think we should add this to that example because they are likely to be used together |
|
This will also need to be changed so that it is only able to be used on "basic" headers |
|
Can you split the logo size change out into its own PR. It makes this PR harder to review because it affects so many pages |
...ence/ds-vr-test__components_header_example-header-with-search-button_0_document_2_mobile.png
Outdated
Show resolved
Hide resolved
src/components/button/_button.scss
Outdated
| &--header-search.active:focus & { | ||
| &__inner { | ||
| padding: 0.875rem !important; | ||
| border: 0; |
There was a problem hiding this comment.
Is this needed? This line seems to make no difference
There was a problem hiding this comment.
@precious-onyenaucheya-ons I don't think this comment has been addressed yet?
There was a problem hiding this comment.
removed
There was a problem hiding this comment.
@precious-onyenaucheya-ons I think this is still in the code?
There was a problem hiding this comment.
This doesn't look like its been removed to me either
The underlines are currently misaligned |
Should this have an inner blue boarder? Not a black one like the search on the Service manual header? |
There was a problem hiding this comment.
Im not sure why this has changed. It looks like the radios at the bottom with the dropdown have changed slightly
There was a problem hiding this comment.
@precious-onyenaucheya-ons have you looked in to this one? Doesn't look major but curious to see if there is a reason for the change
There was a problem hiding this comment.
This one needs investigating
There was a problem hiding this comment.
I've noticed there are a couple of potential issues here.
- There is a gap between the right hand edge of the button and the edge of the header. This is different to the design. Might be ok as it fits with the way the header has been constructed but perhaps worth checking if this is a deal breaker.
- There is no hover state on the button to close the search. It's there to open it, but not there to close it.
- I think the links under
Popular Searchesare meant to stack into a single column when on mobile, rather than wrap as it seems to do currently.
@rmccar, I have updated this to have a black border |
The close search icon isn't black when focused. Should this have a focus state like this because the menu button doesn't? |
Around about 370px width the buttons wrap |
|
When the search is open and you tab through the buttons there is an element that is focused on between the buttons, might be something that we need to remove the focus state on |
|
When you:
The screen looks like nothing is focused but if you can hit enter again to open the search. This should work the same as the menu button where the search button has the focus state when it is closed in this way |
precious-onyenaucheya-ons
force-pushed
the
feature/162-header-search-button
branch
from
4413e72 to
7b2e368
Compare
What is the context of this PR?
ONSDESYS-162
Added a search button to the header component. Clicking the button opens a panel containing a search input and a list of popular searches.
I have also increased the size of the ons logo as part of this ticket to fit the figma design
How to review this PR
Check that example-header-with-search-button has the new search button and has the design as specified in figma
Checklist
This needs to be completed by the person raising the PR.