search

ONSdigital / design-system

ONS Design System
https://service-manual.ons.gov.uk/design-system
MIT License
27 stars 17 forks source link

Fix focus state on neutral header #3220

Open SriHV opened 1 month ago

SriHV commented 1 month ago

What is the context of this PR?

3204...

Three issues are addressed in this PR:

After conversations with Dina and Joe following things were changed:

How to review this PR

Go to 'components/header/example-header-neutral-for-multicoloured-logo' and focus each element to check the changes above mentioned and compare this with the original 'example-header-neutral-for-multicoloured-logo'

Checklist

This needs to be completed by the person raising the PR.

netlify[bot] commented 1 month ago

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

Name Link
Latest commit 76783987e0a4a94ecf630d0f225fbb79205cc17e
Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/668404aabe76cf00081ca7a3
Deploy Preview https://deploy-preview-3220--ons-design-system-preview.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

rmccar commented 1 month ago

The logo looks good but I think we need a bit more information on the other things. I'm not sure the hover line in the header links should be white. Also the sub navigation now no longer shows if it is being hovered and there isn't much differentiation between one that is selected and hovered. Is this ok? Also to me I think the hover line should be closer to the text and not be shown if the item is selected but that doesn't solve the issue in the difference between just selected and hovered and selected. Think this needs some design discussion.

SriHV commented 1 month ago

Had a chat with Joe, white line on the hover needs more investigation along with usage of neutral header. He said he will follow up with Dina on this and let me know again. He is fine with changes made in sub navigation menu items focus, hover and click.

rmccar commented 2 weeks ago

I don't think you have either your husky hooks set up and/or the EditorConfig plugin in your vscode because your css isn't formatted

Sri's comment: This is done. Can you check it now

The changes look good, just checking did you manage to get your husky hooks working or did you do it manually?

SriHV commented 2 weeks ago

I don't think you have either your husky hooks set up and/or the EditorConfig plugin in your vscode because your css isn't formatted Sri's comment: This is done. Can you check it now

The changes look good, just checking did you manage to get your husky hooks working or did you do it manually?

Yess, I have setup husky with Alessio's help and downloaded all the extensions. Some were missing

SriHV commented 1 week ago
Screenshot 2024-06-13 at 09 26 51

Are these dark blue links supposed to still be dark blue?

Sri's Comment: I've checked the links in the original neutral header, there are dark blue links here too. Shall I discuss this with Dina? Screenshot 2024-06-17 at 13 02 10

Had a chat with Dina and changed the color of the links to black image

rmccar commented 5 days ago

Screenshot 2024-06-27 at 13 20 40

Is this account button meant to be black? shouldn't it be white?

SriHV commented 19 hours ago

Screenshot 2024-06-27 at 13 20 40

Is this account button meant to be black? shouldn't it be white?

I have used existing class .ghost-dark here. If we require background colour white when its active, I will have to add a new class for this. Shall I do that?

rmccar commented 19 hours ago

Screenshot 2024-06-27 at 13 20 40 Is this account button meant to be black? shouldn't it be white?

I have used existing class .ghost-dark here. If we require background colour white when its active, I will have to add a new class for this. Shall I do that?

Looking at the other versions of the menu button they don't have a change this big. The other ones seem to just have a small adjustment to the colour because its selected. I would check with Joe and Dina but I don't think this should be black. To match other menu buttons I think it should be a very light grey or maybe white

SriHV commented 16 hours ago

Screenshot 2024-06-27 at 13 20 40 Is this account button meant to be black? shouldn't it be white?

I have used existing class .ghost-dark here. If we require background colour white when its active, I will have to add a new class for this. Shall I do that?

Looking at the other versions of the menu button they don't have a change this big. The other ones seem to just have a small adjustment to the colour because its selected. I would check with Joe and Dina but I don't think this should be black. To match other menu buttons I think it should be a very light grey or maybe white

I checked with Dina. She told to make account button similar to the menu button here. I will create a new class for this image