Add Menu button in new header variant #3478
Conversation
✅ Deploy Preview for ons-design-system-preview ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
SriHV
changed the title
Im not sure if the focus state should look like this, usually our focus states have black underlines. This will need to be checked with a designer |
The links in the menu look like this when focused |
When open and focused on, the menu button looks like this, I know its like this in the prototype but I think this may need to be reviewed by a designer |
https://github.com/ONSdigital/design-system into enhancement/159/add-menu-button-in-new-header-variant
|
After talking to Joe, I've added border to the menu when hovered, focus or active |
This is fixed now |
Is Joe happy with when the menu is open that on the menu button there is no difference when hovering or focusing? |
Yes, he told to match the color of the text. So for hover and active it is --ons-color-text-link-hover and for focus it is --ons-color-black |
When the menu is open the text colour doesn't look like its changing when its hovered or focused to me |
I spoke with Joe, we don’t need the color change—the hover and focus states for the menu button is similar to gov.uk. |
There was a problem hiding this comment.
I think the params need to be reorganised a bit more. I find the structure of them and the names a bit confusing and dont represent what they actually are to the user. Ive made some suggestions which you dont have to go with if you can think of a better way of structuring it but have a think about it
| &--menu { | ||
| align-items: center; | ||
| display: flex; | ||
| height: 67px; |
There was a problem hiding this comment.
I think we should avoid setting the height like this. Find out how the height is set on the header and see if you can inherit that
| border: 0; | ||
| border-radius: 0; |
| border-radius: 0; | ||
| box-shadow: none; | ||
| color: var(--ons-color-text-link); | ||
| font-weight: 700; |
There was a problem hiding this comment.
This is already being set with the ons-u-fs-s--b class
|
|
||
| &--menu:focus & { | ||
| &__inner { | ||
| background: none; |
| margin-bottom: 1rem; | ||
| padding-top: 2.5rem; |
There was a problem hiding this comment.
We can extend utility classes here
| | button | object`<SignOutButton>` | false | Settings for the [sign out button](#signoutbutton) in the header used to exit a transactional service | | ||
| | navigation | array`<Navigation>` | false | Settings for the [main menu links](#navigation) | | ||
| | siteSearchAutosuggest | `Autosuggest` [_(ref)_](/components/autosuggest) | false | Sets the autosuggest functionality in the header | | ||
| | navLinks | object`<ServiceLinks>` | false | Settings for the [menu button navigation](#navLinks) in the masthead | |
There was a problem hiding this comment.
I think nav links is quite close to navigation, we need to differentiate between them because they are different things. Also the object that gets passed in needs to match the name of the object in the documentation (ServiceLinks isn't the object)
| | classes | string | false | Classes to add to the `<nav>` element | | ||
| | ariaLabel | string | false | The `aria-label` attribute added to the `<nav>` element. Defaults to “Menu links navigation”. | | ||
| | ariaListLabel | string | false | The `aria-label` attribute added to the `<ul>` element. Defaults to “Menu links”. | | ||
| | keyLinksList | array`<Item>` | true | Settings for an array of [key list items](#keyitem) for each navigation link | |
There was a problem hiding this comment.
Think this should just be keyLinks
| | ariaLabel | string | false | The `aria-label` attribute added to the `<nav>` element. Defaults to “Menu links navigation”. | | ||
| | ariaListLabel | string | false | The `aria-label` attribute added to the `<ul>` element. Defaults to “Menu links”. | | ||
| | keyLinksList | array`<Item>` | true | Settings for an array of [key list items](#keyitem) for each navigation link | | ||
| | itemsList | array`<Item>` | true | Settings for an array of [list items](#item) for each navigation link | |
There was a problem hiding this comment.
Shouldnt this be linksList? But I actually think this isn't descriptive enough. Maybe it should be
menuLinks > menuGroup > Items I think thats a bit more descriptive to the user what that is referring to
What is the context of this PR?
ONSDESYS-159
Added Menu button in the new header variant "basic". The Menu will have Top section and new Lower section
Topic section will have up to 3 columns. Each column have a header and under it can contain hyperlinks
Lower section will have also have 3 columns and subsequent child items.
The spacing for now is similar to the items in footer component.
Introduced new button variant for the menu button and also followed tech notes left in the ticket
The example is taken from here
Code is similar to the one available here
How to review this PR
Check that example-header-basic has the new menu button and has the design as specified in figma and the prototype
Checklist
This needs to be completed by the person raising the PR.