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

Update focus styles #822

Merged
merged 10 commits into from
May 20, 2022
Merged

Update focus styles #822

merged 10 commits into from
May 20, 2022

Conversation

bkjohnson
Copy link
Contributor

@bkjohnson bkjohnson commented May 18, 2022

Description

Part of department-of-veterans-affairs/vets-design-system-documentation#654

Companion PR: department-of-veterans-affairs/component-library#433

I left review comments, but links that aren't in the header/footer aren't getting the box-shadow styles because they could wrap, and box-shadow focus styling would look bad. Keeping the outline like we currently have (but with a different color) and setting the color to be dark and the background to be light should ensure that a focused link will have good contrast no matter where it appears.

Testing done

yarn link and tested builds in vets-website and content-build

Screenshots

Example of focus styling for buttons, links, and wrapped links where the ends are far apart

Tabbing around on homepage with new box-shadow styling

Example where wrapped links are stacked underneath

Tabbing around in a narrow screen

Acceptance criteria

  • [ ]

Definition of done

  • Changes have been tested in vets-website
  • Changes have been tested in IE11, if applicable
  • Documentation has been updated, if applicable
  • A link has been provided to the originating GitHub issue (or connected to it via ZenHub)
  • No sensitive information (i.e. PII/credentials/internal URLs/etc.) is captured in logging, hardcoded, or specs

packages/formation/sass/base/_b-mixins.scss Outdated Show resolved Hide resolved
@@ -83,11 +83,12 @@ $color-green-light: #4aa564;
$color-gold-lightest: #fff1d2;
$color-gold-lighter: #fad980;
$color-gold-light: #f9c642;
$color-gold-50v: #936F38; // This is something that's not in USWDS 1.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is copied from the USWDS token based on an issue discussion.

packages/formation/sass/base/_va.scss Outdated Show resolved Hide resolved
bkjohnson added a commit to department-of-veterans-affairs/component-library that referenced this pull request May 18, 2022
This should be done before merge. Work is being done here to make it
possible:

- department-of-veterans-affairs/veteran-facing-services-tools#822
We're sticking with the double box-shadow for links in the header &
footer, but going with the outline approach for links in the main
content area due to the possibility of wrapping
Comment on lines -95 to -98
@mixin focus-gold-lighter-highlight {
background: $color-gold-lighter;
outline: 2px solid $color-gold-lighter;
outline-offset: 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this because it was related and it didn't seem used. I saw it was added in 1b95985, but I couldn't find a PR for it. I also did two different git bisects in vets-website so I could try to pin down:

  1. Where the formation version changed to 3.0.0
    • I don't think this was useful because we don't have any release notes tied to versions - I was just going off of the tag on the commit
  2. Where focus-gold-lighter-highlight was used in vets-website.

I didn't find it anywhere but I may have taken a wrong direction in a bisect and missed it, especially if it was around for a short amount of time.

Comment on lines +90 to +92
@mixin focus-gold-vivid-outline {
box-shadow: 0 0 0 3px $color-white, 0 0 4px 6px $color-gold-50v;
outline: none;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing the mixin name means that we'll have to make a few changes in vets-website (and might make this a "breaking change" technically):

I did it since we aren't using the gold-light color anymore and I thought it would be good for the mixin name to not reference the old color.

@@ -206,8 +206,7 @@ $level-3-hover-padding: 8px 12px 8px 30px;
}

&:focus {
outline: 2px solid $color-gold;
outline-offset: 3px;
@include focus-gold-vivid-outline;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is actually used anywhere, but I felt like removing it was out of scope for the PR. I updated it just to keep things consistent.

& .menu-item-container {
& > a {
font-weight: bold;
}
&:hover {
background-color: $color-gray-lightest;
color: $color-blue;
}
&:focus {
outline: 2px solid $color-gold;
outline-offset: 3px;
}
}

Unused in content-build

Unused in vets-website

Unused in component-library react components

It's even unused in the sidenav.js file:

Unused in formation js

@@ -35,7 +35,7 @@ body {
&:focus {
position: inherit;
top: auto;
outline: 2px solid $color-gold;
@include focus-gold-vivid-outline;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for the "Skip to content" link at the top of the page.

@@ -1,6 +1,6 @@
{
"name": "@department-of-veterans-affairs/formation",
"version": "7.0.1",
"version": "8.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The major version change is because the mixin rename means that sass will throw an error when it tries to compile files that are using the old focus-gold-light-outline. Let me know if you think this should be something lower.

This is from a yarn build in vets-website after I linked the local packages:

webpack sass error

Copy link
Contributor

Choose a reason for hiding this comment

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

Major version seems appropriate here 👍

Comment on lines +104 to +108
&:focus {
outline: $focus-outline;
background: $color-white;
color: $color-link-default;
box-shadow: unset;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The gold outline gives us color contrast on light backgrounds, and setting the focused element's background to white and giving it a darker text ensures that we'll have good contrast for links on dark backgrounds.

@bkjohnson bkjohnson marked this pull request as ready for review May 19, 2022 22:42
@bkjohnson bkjohnson requested a review from a team as a code owner May 19, 2022 22:42
@bkjohnson bkjohnson changed the title Focus color fix Update focus styles May 19, 2022
@bkjohnson bkjohnson merged commit 3a6a902 into master May 20, 2022
@bkjohnson bkjohnson deleted the focus-color-fix branch May 20, 2022 14:55
bkjohnson added a commit that referenced this pull request Jun 21, 2022
bkjohnson added a commit that referenced this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants