Add search button to header #3483

precious-onyenaucheya-ons · 2025-01-30T15:13:27Z

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.

I have selected the correct Assignee
I have linked the correct Issue

netlify · 2025-01-30T15:13:43Z

✅ Deploy Preview for ons-design-system-preview ready!

Name	Link
🔨 Latest commit	`3df9796`
🔍 Latest deploy log	https://app.netlify.com/sites/ons-design-system-preview/deploys/67c08db784d2eb000832a6d5
😎 Deploy Preview	https://deploy-preview-3483--ons-design-system-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

admilne

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.

rmccar · 2025-02-13T12:40:58Z

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

rmccar · 2025-02-17T10:57:22Z

This will also need to be changed so that it is only able to be used on "basic" headers

rmccar · 2025-02-20T10:41:34Z

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

rmccar · 2025-02-21T13:35:10Z

...ence/ds-vr-test__components_header_example-header-with-search-button_0_document_2_mobile.png

This screenshot is a a bit of an issue, we don't want them wrapping like that

rmccar · 2025-02-21T13:42:59Z

src/components/button/_macro.njk

@@ -61,6 +61,10 @@
            {% set iconType = "loader" %}
            {% set iconPosition = "after" %}
            {% set variantClasses = " ons-btn--loader ons-js-loader ons-js-submit-btn" %}
+        {% elif 'search-icon' in params.variants %}


Suggested change

{% elif 'search-icon' in params.variants %}

{% elif 'search' in params.variants %}

This should just be search to fit in with the way others are done

rmccar · 2025-02-21T13:54:31Z

src/components/button/_button.scss

+    &--header-search.active:focus & {
+        &__inner {
+            padding: 0.875rem !important;
+            border: 0;


Is this needed? This line seems to make no difference

@precious-onyenaucheya-ons I don't think this comment has been addressed yet?

rmccar · 2025-02-21T13:55:10Z

The underlines are currently misaligned

rmccar · 2025-02-21T13:55:40Z

Underline is missing when the search is open

rmccar · 2025-02-21T13:57:48Z

src/components/button/_button.scss

+    &--header-search:active:focus &,
+    &--header-search.active:focus & {
+        &__inner {
+            padding: 0.875rem !important;


This doesn't seem to need important. If we can we should avoid using important and only use it if we need to. Can you review the places youve added important and check that they are needed

@rmccar I see this hasn't changed. It looks like it is required at the moment because there is some css later on in the file which overrides the padding needed here. On line 780 in _button.scss you have the following:

.ons-search__btn { height: 92.5%; //this is to allow the button to be fully aligned with the input by accounting for the shadow box of 3px .ons-btn__inner:has(> .ons-icon) { padding-right: 0.75rem; height: 100%; } }

This looks a little odd to me, anyone know if this is required? But this is the style resulting in @precious-onyenaucheya-ons needing to use !important. It's more specific and comes later in the file

rmccar · 2025-02-21T13:58:05Z

src/components/button/_button.scss

+            padding: 0.875rem !important;
+            border: 0;
+            border-radius: 0;
+            box-shadow: none !important;


Important doesnt seem to be needed here too

rmccar · 2025-02-21T13:59:54Z

Search icon not turning black when focused on

rmccar · 2025-02-21T14:11:28Z

Should this have an inner blue boarder? Not a black one like the search on the Service manual header?
If it is meant to be blue I'm not sure there should be a black border between the yellow and blue

rmccar · 2025-02-21T14:36:29Z

...nce/ds-vr-test__patterns_correct-errors_example-errors-proto-errors_0_document_0_desktop.png

Im not sure why this has changed. It looks like the radios at the bottom with the dropdown have changed slightly

@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

admilne · 2025-02-26T12:55:58Z

src/components/header/_macro-options.md

+| id                 | string                 | true     | The HTML `id` of the `search button` element. Used for the `aria-controls` attribute for the search toggle button displayed on small viewports. |
+| classes            | string                 | false    | Classes to add to the `search button` element                                                                                                   |
+| ariaLabel          | string                 | false    | The `aria-label` attribute added to the `search button` element. Defaults to Search navigation”.                                                |
+| popularSearches    | array`<SearchItem>`    | true     | Settings for an array of [popular searches](#searchitem) for each search link                                                                   |


I got the impression that the title of Popular Searches could be configurable. Might need to check on that. If the title is configurable, this may need a better name.

admilne · 2025-02-26T12:56:48Z

src/components/header/_macro-options.md

@@ -18,6 +18,7 @@
 | navigation            | array`<Navigation>`                               | false                                                       | Settings for the [main menu links](#navigation)                                                                                                |
 | siteSearchAutosuggest | `Autosuggest` [_(ref)_](/components/autosuggest)  | false                                                       | Sets the autosuggest functionality in the header                                                                                               |
 | menuLinks             | object`<MenuLinks>`                               | false                                                       | Settings for the [menu button navigation](#menuLinks) in the masthead                                                                          |
+| searchLinks           | object`<SearchLinks>`                             | false                                                       | Settings for the [search button navigation](#searchLinks) in the masthead                                                                      |


Should there be something here to say you can only have 5 search links?

admilne · 2025-02-26T12:57:59Z

src/components/header/_macro-options.md

+
+| Name | Type   | Required | Description                           |
+| ---- | ------ | -------- | ------------------------------------- |
+| text | string | false    | The text for the popular search item. |


'popular search' may need to be renamed if the title of Popular Searches is configurable

admilne · 2025-02-26T12:58:07Z

src/components/header/_macro-options.md

+| Name | Type   | Required | Description                           |
+| ---- | ------ | -------- | ------------------------------------- |
+| text | string | false    | The text for the popular search item. |
+| url  | string | true     | The URL for the popular search item   |


'popular search' may need to be renamed if the title of Popular Searches is configurable

admilne · 2025-02-26T12:59:11Z

src/components/header/_macro.njk

+                    </div>
+                    {% if params.searchLinks.popularSearches %}
+                        <div class="ons-container">
+                            <h2 class="ons-u-fs-r--b ons-u-mb-s ons-header-nav-search__heading">Popular Searches</h2>


I get the feeling Popular Searches should be configurable based on the information within the ticket. Might need to check on this.

admilne · 2025-02-26T13:00:13Z

src/components/header/_macro.njk

+                >
+                    <div class="ons-container">
+                        <div class="ons-header-nav-search__input">
+                            {# <h2 class="ons-u-fs-r--b ons-u-mb-s ons-header-nav-search__heading">Search the ONS</h2> #}


Can this commented out code be removed?

admilne · 2025-02-26T13:00:59Z

src/components/header/example-header-with-search-button.njk

+                "text": 'Search',
+                "ariaLabel": 'Toggle search'
+            },
+            "popularSearches": [


Again, this may need to be changed if the title of Popular Searches is configurable

admilne

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 Searches are meant to stack into a single column when on mobile, rather than wrap as it seems to do currently.

precious-onyenaucheya-ons added 2 commits January 30, 2025 15:15

add initial changes

b021a18

update styling and approve visual test

7bf2beb

precious-onyenaucheya-ons force-pushed the feature/162-header-search-button branch from 5a42c21 to 7bf2beb Compare January 30, 2025 15:18

precious-onyenaucheya-ons self-assigned this Jan 30, 2025

precious-onyenaucheya-ons added the Enhancement Change of existing feature label Jan 30, 2025

precious-onyenaucheya-ons added 3 commits January 30, 2025 19:55

limit popular searches

4d81fc4

rename class

b0eb10b

remove redundant styling

586e897

precious-onyenaucheya-ons marked this pull request as ready for review January 31, 2025 08:45

precious-onyenaucheya-ons requested a review from a team as a code owner January 31, 2025 08:45

admilne requested changes Feb 5, 2025

View reviewed changes

precious-onyenaucheya-ons and others added 6 commits February 6, 2025 13:09

Merge branch 'main' into feature/162-header-search-button

672fa08

update styling

6b43a75

update onslogo size

b375a29

more styling update

7fb0698

Merge branch 'main' into feature/162-header-search-button

289b12b

update the search button

700de62

precious-onyenaucheya-ons added 3 commits February 17, 2025 17:19

fix tests

f6c0fef

approve visual tests

5378d07

fix toggle

4cc084c

precious-onyenaucheya-ons and others added 2 commits February 21, 2025 11:20

Merge branch 'main' into feature/162-header-search-button

b1bf012

revert changes to the ons icon

b3b5c22

rmccar reviewed Feb 21, 2025

View reviewed changes

update styling and visual tests

82fc26d

rmccar reviewed Feb 21, 2025

View reviewed changes

precious-onyenaucheya-ons and others added 2 commits February 21, 2025 17:51

address PR comments

8335a2e

Merge branch 'main' into feature/162-header-search-button

0a4cfe4

admilne reviewed Feb 26, 2025

View reviewed changes

admilne requested changes Feb 26, 2025

View reviewed changes

precious-onyenaucheya-ons added 2 commits February 26, 2025 14:20

address pr comments

446da04

update focus state

3df9796

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search button to header #3483

Add search button to header #3483

precious-onyenaucheya-ons commented Jan 30, 2025 •

edited

Loading

netlify bot commented Jan 30, 2025 •

edited

Loading

admilne left a comment

rmccar commented Feb 13, 2025

rmccar commented Feb 17, 2025

rmccar commented Feb 20, 2025

rmccar Feb 21, 2025

rmccar Feb 21, 2025

rmccar Feb 21, 2025

admilne Feb 26, 2025

rmccar commented Feb 21, 2025

rmccar commented Feb 21, 2025

rmccar Feb 21, 2025 •

edited

Loading

admilne Feb 26, 2025

rmccar Feb 21, 2025

rmccar commented Feb 21, 2025

rmccar commented Feb 21, 2025 •

edited

Loading

rmccar Feb 21, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne Feb 26, 2025

admilne left a comment

	{% elif 'search-icon' in params.variants %}
	{% elif 'search' in params.variants %}

Add search button to header #3483

Are you sure you want to change the base?

Add search button to header #3483

Conversation

precious-onyenaucheya-ons commented Jan 30, 2025 • edited Loading

What is the context of this PR?

How to review this PR

Checklist

netlify bot commented Jan 30, 2025 • edited Loading

✅ Deploy Preview for ons-design-system-preview ready!

admilne left a comment

Choose a reason for hiding this comment

rmccar commented Feb 13, 2025

rmccar commented Feb 17, 2025

rmccar commented Feb 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmccar commented Feb 21, 2025

rmccar commented Feb 21, 2025

rmccar Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmccar commented Feb 21, 2025

rmccar commented Feb 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

admilne left a comment

Choose a reason for hiding this comment

precious-onyenaucheya-ons commented Jan 30, 2025 •

edited

Loading

netlify bot commented Jan 30, 2025 •

edited

Loading

rmccar Feb 21, 2025 •

edited

Loading

rmccar commented Feb 21, 2025 •

edited

Loading