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

Feat: Focus rings #1633

Merged
merged 10 commits into from
Aug 11, 2023
Merged

Feat: Focus rings #1633

merged 10 commits into from
Aug 11, 2023

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Aug 9, 2023

closes #1387

What this PR does

  • Add custom yellow focus rings to: all buttons, links, form text input, radio input, card link
  • Add custom white focus ring to: footer link
  • Removes unused button class

Specs

  • Color: #fdb714 or #fafafa
  • Outline: 3px solid outline (except for the radio input, it's 2px)
  • Outline offset (the space between the element and the outline): 2px (except for the radio input, it's 1px)
  • Uses the border-radius of the element itself, like for card, the outline and the element's border-radius are both 8. For text links, the border-radius is set to 2px.
  • For card, there is an extra box-shadow.
  • For text input, matched the amount of margin/padding between the bottom of the input and the beginning of the helper text - to create more space for the focus ring.

What it looks like

Focus rings are activated by the keyboard tab, so they're usually seen on Desktop mode.

image image image image image image image image image image image image

@machikoyasuda machikoyasuda requested a review from a team as a code owner August 9, 2023 21:59
@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates labels Aug 9, 2023
@machikoyasuda machikoyasuda self-assigned this Aug 9, 2023
@machikoyasuda machikoyasuda added this to the Veterans milestone Aug 9, 2023
@machikoyasuda
Copy link
Member Author

@srhhnry Once this PR is reviewed, approved and merged, you'll be able to test the focus rings on dev-benefits.calitp.org/

@srhhnry
Copy link

srhhnry commented Aug 9, 2023

Sounds great! I was having trouble with codepen (couldn't keep the focus state sustained, as I think it was a click action, so plugins were having a hard time). But all these screenshots look good.

Copy link
Member

@angela-tran angela-tran 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 really good @machikoyasuda! I have one question about the radio input focus ring and noticed a bug that we could fix as a part of this PR.

@@ -580,6 +608,12 @@ footer .footer-links li a:visited {
box-shadow: inset 0 0 0 var(--border-width) var(--bs-white);
}

.radio-input:focus,
.radio-input:focus-visible {
outline: 2px solid var(--focus-color) !important;
Copy link
Member

Choose a reason for hiding this comment

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

I noticed the radio input focus rings are skinnier than everything else's focus rings. Should this be 3px?

What I see locally

  • Radio button focus ring image
  • Compared to the link focus ring
    image

Set to 3px

image

Figma

image

Copy link
Member Author

Choose a reason for hiding this comment

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

It's hard to figure out the width of the yellow circle's stroke. But I got the 2px from this:
image

<svg width="30" height="30" viewBox="0 0 30 30" fill="none" xmlns="http://www.w3.org/2000/svg">
  <circle cx="15" cy="15" r="8" fill="#323A45"/>
  <circle cx="15" cy="15" r="11" stroke="#323A45" stroke-width="2"/>
  <circle cx="15" cy="15" r="14" stroke="#FDB714" stroke-width="2"/>
</svg>

This line specifically:
<circle cx="15" cy="15" r="14" stroke="#FDB714" stroke-width="2"/>

The yellow circle has a stroke-width of 2px, so that's why I set the radio button focus ring to 2px.

@srhhnry Should the focus ring for radio input be 2px (matches Figma) or 3px (matches all the other rings)?

Copy link
Member

Choose a reason for hiding this comment

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

@machikoyasuda Hmm, yeah I see how the Figma says the radio input border should be 2px and all the others are 3px. I wonder why 2px is causing it to look so narrow on the radio input.

Copy link
Member

Choose a reason for hiding this comment

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

Should the focus ring for radio input be 2px (matches Figma) or 3px (matches all the other rings)?

@srhhnry Just to clarify this question, I don't think we're questioning the use of 2px in Figma for the radio input, but rather it seems like for some reason Figma and the browser are not showing 2px the same way. So I think what we're trying to decide is if we should just use 2px even though it doesn't look quite like Figma or use 3px even though that's not what Figma says.

Copy link
Member

@angela-tran angela-tran Aug 11, 2023

Choose a reason for hiding this comment

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

@machikoyasuda Maybe we just merge this in with 2px so Sarah (or whoever on the design team 😄) can take a look at it on the dev site and decide if it looks too narrow?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yea - I'd also bet if we put our Mac vs Windows machines side by side, it might look a lil different between our machines. The input is gonna look different from Figma cos the Figma input was created from scratch. And your screenshots look a lil different from my Mac/Firefox rendering too.

Copy link
Member Author

Choose a reason for hiding this comment

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

image on Mac/Safari image on Mac/Firefox

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. Ok, I feel good about leaving this as 2px and having design look at it later, either as a part of a design QA or whenever design has time.

benefits/core/templates/core/base.html Outdated Show resolved Hide resolved
benefits/core/templates/core/base.html Outdated Show resolved Hide resolved
@machikoyasuda machikoyasuda merged commit cf65fad into dev Aug 11, 2023
@machikoyasuda machikoyasuda deleted the feat/1387-focus-rings branch August 11, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployment-dev [auto] Changes that will trigger a deploy if merged to dev front-end HTML/CSS/JavaScript and Django templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update focus ring
3 participants