Skip to content

fix(navigation): improve focus style to meet WCAG 2.2 AA#1620

Merged
dancormier merged 3 commits intodevelopfrom
STACKS-538/focus-styles-navigation
Jan 30, 2024
Merged

fix(navigation): improve focus style to meet WCAG 2.2 AA#1620
dancormier merged 3 commits intodevelopfrom
STACKS-538/focus-styles-navigation

Conversation

@dancormier
Copy link
Copy Markdown
Contributor

@dancormier dancormier commented Jan 29, 2024

Resolves STACKS-538

@netlify
Copy link
Copy Markdown

netlify Bot commented Jan 29, 2024

Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 698097f
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/65b91806098efe00083b2281
😎 Deploy Preview https://deploy-preview-1620--stacks.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.

Copy link
Copy Markdown
Collaborator

@CGuindon CGuindon left a comment

Choose a reason for hiding this comment

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

Overall rings look good. I just noticed that in HC Dark mode the inner ring keeps the slighter slighter background color we're using in regular Dark mode. Since we have a darker background for the site in HC mode, the inner ring should also change and match that. Can the inner color change based on HC or regular for dark mode?

I also assume this might be the case for all the other components. Something I missed when reviewing the others.

If we can only pick 1 color (since light mode just has white in both HC and regular) than I would opt to use the darker one to get more contrast with the adjacent blue.

Screenshot 2024-01-30 at 7 29 40 AM

@dancormier
Copy link
Copy Markdown
Contributor Author

@CGuindon nice catch! If we use --white, the ring matches the background color in all cases. I've made the change in this PR, and I've removed the change to topbar for now so I can handle all of the topbar focus states at the same time.

- Light Dark
Default 20240130-104046 20240130-103945
HC 20240130-103953 20240130-103959

Copy link
Copy Markdown
Contributor

@giamir giamir left a comment

Choose a reason for hiding this comment

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

This looks good. I understand navigation-item in the context of the topbar will be handled separately. I also found it interesting that the topbar override the focus ring behavior of navigation-item. Is there any reason we do that?

.s-topbar .s-navigation .s-navigation--item:focus-visible {
    box-shadow: var(--theme-topbar-search-shadow-focus, 0 0 0 var(--su-static4)) var(--focus-ring));
}

Could we rely only on the focus-visible style of the navigation-item in the topbar? Just trying to understand if we can delete extra code. 😀

@dancormier
Copy link
Copy Markdown
Contributor Author

This looks good. I understand navigation-item in the context of the topbar will be handled separately. I also found it interesting that the topbar override the focus ring behavior of navigation-item. Is there any reason we do that?

.s-topbar .s-navigation .s-navigation--item:focus-visible {
    box-shadow: var(--theme-topbar-search-shadow-focus, 0 0 0 var(--su-static4)) var(--focus-ring));
}

Could we rely only on the focus-visible style of the navigation-item in the topbar? Just trying to understand if we can delete extra code. 😀

Navigation within the topbar includes some custom theme that we'll need to figure out how to deal with. We'll have the following options:

  1. Remove the custom styling altogether and just rely on the default styling
  2. Keep the custom styling but apply it in a way that matches the navigation focus styling

There are other ways we could deal with it, but those are the two best options IMO. I originally included topbar modifications in this PR but changed my mind and figured we should deal with those when we focus on the topbar.

Related: I'm currently in the process of writing visual tests for topbar so we can more safely change the component. I've also opened a new branch to update the topbar to use PPCPs which should benefit from the baseline test images if we can get them merged in soon.

@dancormier dancormier merged commit 2909789 into develop Jan 30, 2024
@dancormier dancormier deleted the STACKS-538/focus-styles-navigation branch January 30, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants