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

Add focus outline back to default buttons #473

Merged
merged 2 commits into from
Aug 2, 2019
Merged

Conversation

yvonnetangsu
Copy link
Member

@yvonnetangsu yvonnetangsu commented Aug 1, 2019

READY FOR REVIEW

Summary

  • Put focus outline back on default buttons and let developers remove them in their own projects if needed, since it's really difficult to put outline back once it's been removed (no good CSS code to reset it). Having the outline as default helps with accessibility.
  • Also remove extra bottom margin on some of the CTA variants.

Needed By (Date)

  • Whenever

Urgency

  • N/A

Steps to Test

  1. Pull this branch and compile styleguide
  2. Go to the Components -> button section and tab through the buttons, check that the browser default outline ring is back when button is on focus.
  3. Look at the Site Search component and check that the magnifying glass icon still has outline when tab to it.
  4. Check Main Nav and Masthead component at MD breakpoint or smaller. See that the hamburger menu toggle button has no outline on focus (this is an exception case that has no focus outline since it has the thick border underneath already).

Affected Projects or Products

  • Decanter
  • Redwood WP theme

Associated Issues and/or People

… menu buttons; remove margin bottom on h2 button on CTA; remove non-prefixed "appearance: none" since it's non valid
@yvonnetangsu yvonnetangsu requested a review from JBCSU August 1, 2019 21:40
core/src/scss/elements/input/_input.scss Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@

.su-cta__button {
@include button-big;
@include margin(null null 0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this necessary? Just curious.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the current one, when you tab to the CTA, in a couple of them, there's a white space the whole CTA (more obvious when you tab to it so you see the focus outline).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, I see why you want to remove the bottom margin on the CTA button. But this has the effect of changing the vertical spacing of the entire CTA. Should we not just move the bottom margin to the whole CTA, vs. eliminating it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we don't actually want any external spacing that's outside any Decanter elements (no spacing outside the top level wrapper) so I'm removing it all together.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm. Yes, I agree with that in principle. However, the fact is we did effectively have external spacing on the CTA component. If people upgrade to the latest master, their layout will be affected if they use CTA's. Maybe that's ok, but I want to make a conscious decision about it.

@sherakama, your thoughts? Are CTA's used on any of your decanter-based sites? Is SWS okay with the bottom margin going away?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point about backward compatibility 😄

Copy link
Member

@sherakama sherakama Aug 2, 2019

Choose a reason for hiding this comment

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

CSS can change and the styles applied to elements can change, esp, if the new code keeps the end result the same without changing or removing variables mixins, or placeholders. Keep in mind, we try to avoid top margin and try to only apply margin to the bottom of elements to stay consistent.

Copy link
Member

@sherakama sherakama left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

Copy link
Collaborator

@JBCSU JBCSU left a comment

Choose a reason for hiding this comment

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

Looks great! GTG

@JBCSU JBCSU merged commit 50ed3f4 into master Aug 2, 2019
@JBCSU JBCSU deleted the 472-button-outline branch August 2, 2019 18:00
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