Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix focus state on neutral header #3220

Merged
merged 35 commits into from
Jul 15, 2024

Conversation

SriHV
Copy link
Contributor

@SriHV SriHV commented May 29, 2024

What is the context of this PR?

#3204...
Three issues are addressed in this PR:

  • Text in focus state is black instead of white in header.
  • Single line is displayed in focus state instead of a double line when navigating through menu items.
  • ONS logo is now displayed in focus state

After conversations with Dina and Joe following things were changed:

  • The background color of the neutral header is now white instead of black.
  • The color of the logo links is now black.
  • The focus state of the logo is consistent with the other header examples.
  • The focus state of the links is now black.
  • The area around the header title and navigation menu has a grey background (--ons-color-grey-75), and the text color is white.
  • The focus state of the menu items is now white with an underline in black.Hover state underline is white.

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.

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

Copy link

netlify bot commented May 29, 2024

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

Name Link
🔨 Latest commit de9bb00
🔍 Latest deploy log https://app.netlify.com/sites/ons-design-system-preview/deploys/6694e2569856450008050b19
😎 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.

@SriHV SriHV changed the title Fix/3204/fix focus state on neutral header Fix focus state on neutral header May 29, 2024
@SriHV SriHV added the Bug Something isn't working label May 29, 2024
@SriHV SriHV linked an issue May 29, 2024 that may be closed by this pull request
@SriHV SriHV requested a review from a team May 29, 2024 09:32
@SriHV SriHV marked this pull request as ready for review May 29, 2024 09:32
@rmccar
Copy link
Contributor

rmccar commented May 30, 2024

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.

Copy link
Contributor

@alessioventuriniAND alessioventuriniAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy to approve based on the fact that you still need to get this reviewed by Design :)

@SriHV
Copy link
Contributor Author

SriHV commented May 31, 2024

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.

@SriHV SriHV marked this pull request as draft June 4, 2024 08:30
@SriHV SriHV marked this pull request as ready for review June 11, 2024 10:43
@alessioventuriniAND alessioventuriniAND added the Enhancement Change of existing feature label Jun 12, 2024
Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@alessioventuriniAND alessioventuriniAND left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the expected behaviour?

Can you also confirm if you had your latest work reviewed by Dina or Joe?

Screen.Recording.2024-06-14.at.11.28.28.mov

Sri's Comment:

Yes, Dina asked these changes.

  • White underline for menu items has been made consistent with the design system's navigational items.
  • There is a double underline when focused and hovered

@alessioventuriniAND alessioventuriniAND self-requested a review June 14, 2024 10:31
@SriHV
Copy link
Contributor Author

SriHV commented Jul 2, 2024

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
Copy link
Contributor

rmccar commented Jul 2, 2024

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
Copy link
Contributor Author

SriHV commented Jul 2, 2024

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

@rmccar
Copy link
Contributor

rmccar commented Jul 11, 2024

When the new menu button is open the text changes to blue

Screenshot 2024-07-11 at 12 44 12

@rmccar
Copy link
Contributor

rmccar commented Jul 11, 2024

Screenshot 2024-07-11 at 14 03 26

Just wanted to ask the question because the design has changed quite a bit since the ticket was written but do we still want the green in the logo? Because we usually use either an all black logo or a colour logo which is green and blue. Might need to check this with the designers

SriHV and others added 2 commits July 11, 2024 15:45
@rmccar rmccar removed the Enhancement Change of existing feature label Jul 11, 2024
SriHV and others added 3 commits July 11, 2024 16:22
Co-authored-by: rmccar <42928680+rmccar@users.noreply.github.com>
@rmccar
Copy link
Contributor

rmccar commented Jul 12, 2024

I think we should rename the example from example-header-neutral-for-multicoloured-logo to example-header-neutral

@alessioventuriniAND alessioventuriniAND merged commit e1bc753 into main Jul 15, 2024
9 checks passed
@alessioventuriniAND alessioventuriniAND deleted the fix/3204/fix-focus-state-on-neutral-header branch July 15, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Focus state on neutral header variant is not accessible
4 participants