-
Notifications
You must be signed in to change notification settings - Fork 841
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
[Amsterdam] Unify focus states #4242
Conversation
- Breadcrumbs (not great) - Button groups (still missing on single-select) - Tabs - Links
# Conflicts: # src/themes/eui-amsterdam/global_styling/mixins/_index.scss
Preview documentation changes for this PR: https://eui.elastic.co/pr_4242/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4242/ |
- Fixes up the checkbox,radio,switch outlines - Reduces opacity of focus backgrounds - Fixes link color in Ams. dark mode
Preview documentation changes for this PR: https://eui.elastic.co/pr_4242/ |
# Conflicts: # src/components/datagrid/column_sorting.tsx
@@ -31,7 +31,6 @@ $euiPageBackgroundColor: tintOrShade($euiColorLightestShade, 50%, 30%) !default; | |||
$euiTextSubduedColor: makeHighContrastColor($euiColorMediumShade) !default; | |||
$euiTitleColor: shadeOrTint($euiTextColor, 50%, 0%) !default; | |||
$euiLinkColor: $euiColorPrimary !default; | |||
$euiFocusBackgroundColor: tintOrShade($euiColorPrimary, 90%, 50%) !default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This just got moved to the _states.scss
file
Preview documentation changes for this PR: https://eui.elastic.co/pr_4242/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in Chrome, Safari, Edge, and Firefox and it looks good. 🎉
@@ -40,6 +40,7 @@ | |||
&:focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'll agree that will be a good tweak. But I'd like to push that off to when the Tabs are specifically tackled for Amsterdam. Since right now that would require a new Amsterdam specific overrides file.
This PR attempts to unify and increase the visibility of the focus ring, but only during keyboard use.
Safari and Firefox already strictly use the
:focus
pseudo-class to show focus rings (outlines) when tabbing and not when using the mouse. However, to restrict to keyboard navigation in Chrome, we have to use the new-ish:focus-visible
pseudo-class.Chrome's
auto
style ofoutline
is much different than Firefox and Safari and allows for coloring of the outline while keeping the "rounded" corners/white outer line for contrast. But Firefox and Safari do no, so they'll visibly show just the straight corner version.Buttons example:
Reset file
I first had to duplicate the
_reset.scss
file for Amsterdam and then I undid the blanket removal of the browser'soutline
attribute and only removed it IF the focus is not visible. I also set the outline-color to be the same as the element's text so that it should always have the same contrast level as the text. And changed the position (offset) so it is centered on the elements edge.Focus ring tokens / mixins
Then I changed the
euiFocusRing()
mixin to align more towards using the browser'soutline
property. I had to add a couple extra parameters that are strictly for use in Amsterdam for this new functionality. I also removed thefaux
(box-shadow) version and leaned into the fact that we can't control the border-radius of the outline. I think this is an ok compromise since the focus ring will now only show on keyboard navigation.I also created a couple generic interactive state mixins like
euiFocusState()
,euiHoverState()
andeuiInteractiveStates()
to add all of them. I haven't applied them everywhere, but this gives a a starting point to convert some of the manual styles so we have better consistency.Components
There are still a few components that aren't working quite well and will need follow up but I didn't want to wait on getting this initial set in.
Checklist
[ ] Checked in mobile[ ] Props have proper autodocs[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples[ ] Added or updated jest tests[ ] Checked for breaking changes and labeled appropriately