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

Make EuiBasicTable rows keyboard-accessible when they are clickable. #1206

Merged

Conversation

cjcenizal
Copy link
Contributor

@cjcenizal cjcenizal commented Sep 21, 2018

Summary

When a row has an onClick applied to it, the row becomes keyboard-accessible (you can tab to it) and has a visual focus state.

table_keyboard_accessibility

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Nice, so clean! LGTM, code review only.


.euiTableRow-isClickable {
&:focus {
outline: solid 3px transparentize($euiColorPrimary, .9);
Copy link
Contributor

@cchaos cchaos Sep 24, 2018

Choose a reason for hiding this comment

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

Is there a specific reason why an outline was added? The background color is already changing and the outline (which goes outside of the row borders) makes the coloring look a bit messy. I'd stick to one or the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope I didn't even think about it -- just copied what we had from somewhere else! Which one do you think we should go with?

Copy link
Contributor

Choose a reason for hiding this comment

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

I played around with it some and because of the row borders, the focus ring doesn't look so good, but I would like to adjust the hover color for clickable rows as well, so the best I came up with was:

.euiTableRow-isClickable {
  &:hover {
    background-color: transparentize($euiColorPrimary, .95);
  }

  &:focus {
    background-color: transparentize($euiColorPrimary, .9);
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I like what you did there. This gibes with @chandlerprall's comment too. Will update.

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.

Code looks good to me. Share @cchaos' comment on the styling, but other than that this is really nice!

@cjcenizal
Copy link
Contributor Author

@cchaos @snide Another thing to note is the hover state currently does not override the selected state. Do you think this makes sense or do you prefer the hover state override the selected state?

@chandlerprall
Copy link
Contributor

chandlerprall commented Sep 24, 2018

IMO the hover state shouldn't override, as you can still Enter or Space to trigger the keyboard-selected row.

@cjcenizal
Copy link
Contributor Author

Thanks for the reviews @chandlerprall @snide @cchaos! Updated with your feedback.

@@ -9,6 +9,8 @@ $euiTableActionsAreaWidth: $euiSizeXXL;

// Colors

$euiTableHoverClickableColor: transparentize($euiColorPrimary, .95);
$euiTableFocusClickableColor: transparentize($euiColorPrimary, .9)
Copy link
Contributor

@cchaos cchaos Sep 24, 2018

Choose a reason for hiding this comment

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

Missing semicolon;

Sorry, being picky here, but can you move these to the bottom of the list since they are more specific modifiers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No prob, done.

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.

Thanks!

@cjcenizal cjcenizal merged commit 58b2a99 into elastic:master Sep 24, 2018
@cjcenizal cjcenizal deleted the basic-table-keyboard-accessibility branch September 24, 2018 19:37
@snide snide mentioned this pull request Oct 3, 2018
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.

4 participants