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.

Sign up for GitHub

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

Add focus outline back to default buttons #473

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e6c4858
Add focus outline back to default buttons but remove it from main nav…
yvonnetangsu Aug 1, 2019
2e26957
Remove appearance property from input and button-base
yvonnetangsu Aug 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions core/dist/css/decanter.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions core/src/scss/components/main-nav/_main-nav.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,7 @@
display: flex;
flex-direction: column-reverse;
align-items: center;
outline: none;
width: 40px;
background-color: transparent;
color: color(h-link, $nav-colors);
Expand Down
5 changes: 0 additions & 5 deletions core/src/scss/components/site-search/_site-search.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,4 @@
&:focus {
box-shadow: none;
}

&:focus {
outline: #aaa solid 3px;
outline: -webkit-focus-ring-color auto 5px;
}
}
6 changes: 2 additions & 4 deletions core/src/scss/elements/input/_input.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
input,
textarea,
select {
appearance: none;
-moz-appearance: none;
yvonnetangsu marked this conversation as resolved.
Show resolved Hide resolved
-webkit-appearance: none;
border: $input-border-width solid $color-driftwood;
border-radius: 0;
box-sizing: border-box;
Expand Down Expand Up @@ -41,9 +42,6 @@ textarea {
}

select {
-moz-appearance: none;
-webkit-appearance: none;
appearance: none;
background-color: $color-white;
background: url("#{$image-path}/arrow-down.png");
background-image: url("#{$image-path}/arrow-down.svg");
Expand Down
1 change: 0 additions & 1 deletion core/src/scss/utilities/placeholders/components/_button.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
font-size: $base-font-size;
font-weight: $font-regular;
line-height: 1;
outline: none;
text-align: center;
text-decoration: none;
width: 100%;
Expand Down
1 change: 1 addition & 0 deletions core/src/scss/utilities/placeholders/components/_cta.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

@include grid-media('xs') {
width: 100%;
z-index: 100;
Expand Down