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

use euiFocusBackgroundColor for EuiFacetButton :focus #2365

Merged
merged 7 commits into from
Sep 25, 2019

Conversation

hbharding
Copy link
Contributor

@hbharding hbharding commented Sep 23, 2019

Summary

To date, most usage of EuiFacetButton in Kibana has been done on a white background. With the upcoming Integrations project, we have plans to include this against a light grey background ($euiPageBackgroundColor). The button's focus state is difficult to see when on an off-white background. This PR modifies EuiFacetButton's :focus state to use a light blue background instead of light grey. We use this color elsewhere in EUI such as when focusing on tabs.

Screenshots

Before

image

After

image

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@hbharding
Copy link
Contributor Author

I wasn't sure if I needed to complete everything in the Checklist for a simple 1 liner CSS change.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

I know this is small, but don't forget to add a changlog item. Just add a new line under master. Then you should be safe to merge. As a suggestion, If you wanted to be clever, my guess is that you could use a CSS outline to mimic "padding" so that the focus state wasn't flush to the text. This could be more trouble than it's worth, but it's a trick i've used when the focused element looked weird because of the lack of padding.

https://github.com/elastic/eui/blob/master/CHANGELOG.md

background-color: $euiColorLightestShade;
background-color: $euiFocusBackgroundColor;
// use box-shadow as a "faux outline" to apply left/right padding only
box-shadow: -4px 0 $euiFocusBackgroundColor, 4px 0 $euiFocusBackgroundColor;
Copy link
Contributor

Choose a reason for hiding this comment

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

We also have variables for pixel values that you'll want to use

Suggested change
box-shadow: -4px 0 $euiFocusBackgroundColor, 4px 0 $euiFocusBackgroundColor;
box-shadow: (-$euiSizeXS) 0 $euiFocusBackgroundColor, $euiSizeXS 0 $euiFocusBackgroundColor;

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

@cchaos
Copy link
Contributor

cchaos commented Sep 24, 2019

Oh whoops, looks like you've got a CL conflict. Be sure to rebase before merging.

@hbharding
Copy link
Contributor Author

Ok I think this is ready!

One note: when testing in Firefox I noticed a dotted grey border while tab focusing (see screenshots below). Since this behavior occurs on multiple EUI components and not just EuiFacetButton, we may want to create a separate issue if we want to "normalize" this style. Perhaps something like:

# normalize.scss
button::moz-inner-focus { border: 0; }

image
image

@hbharding hbharding merged commit a92a047 into elastic:master Sep 25, 2019
@hbharding hbharding deleted the EuiFacetButton-focus branch September 25, 2019 16:59
snide pushed a commit to snide/eui that referenced this pull request Oct 10, 2019
* use  for EuiFacetButton :focus

* Updated changelog

* removed trailing whitespace

* fixed indentation

* fixed my mistake / leftover code

* use euiSizeXS instead of px value
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.

3 participants