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 aria-label attributes to indexed fields table, and use EuiIconTip #17427

Merged
merged 4 commits into from
Mar 28, 2018

Conversation

jen-huang
Copy link
Contributor

@jen-huang jen-huang commented Mar 27, 2018

Fixes #17410
Fixes #17411

Improve accessibility of indexed fields table by adding aria-label to the boolean indicators (green dots), and replace <EuiToolTip><EuiIcon/></EuiToolTip> with <EuiIconTip/>.

Bump EUI to 0.0.35 to support color prop for EuiIconTip.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang jen-huang removed the WIP Work in progress label Mar 27, 2018
@jen-huang jen-huang changed the title [WIP] Add aria-label attributes to indexed fields table, and use EuiIconTip Add aria-label attributes to indexed fields table, and use EuiIconTip Mar 27, 2018
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Besides the two suggestions this looks good to me.

<EuiIconTip
type="clock"
color="primary"
aria-label="Info"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather suggest using a short version of the description here as a label, like "Primary time field".
That way a screenreader would read out:

🗣️ Primary time field -pause- This field represents the time that events occurred.
vs:
🗣️ Info -pause- This field represents the time that events occurred.

That way a screen reader user once familiar to what the time field means, can immediately jump to the next element, once she heard "primary time field", instead of needing to wait for the long explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense! updating

<EuiIconTip
type="alert"
color="warning"
aria-label="Warning"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, I would suggest using something like "Multiple type field" instead of "Warning".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense! updating

Copy link
Contributor

@chrisronline chrisronline left a comment

Choose a reason for hiding this comment

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

LGTM! Tim's comments totally make sense and not something I would have caught! I'd definitely second them!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@jen-huang jen-huang merged commit ef4d262 into elastic:master Mar 28, 2018
jen-huang added a commit to jen-huang/kibana that referenced this pull request Mar 28, 2018
…elastic#17427)

* Add aria-label attributes to indexed fields table, and use EuiIconTip
* Use EUI 0.0.35
* Add `Apache License, Version 2.0` to list
jen-huang added a commit that referenced this pull request Mar 28, 2018
…#17427) (#17436)

* Add aria-label attributes to indexed fields table, and use EuiIconTip
* Use EUI 0.0.35
* Add `Apache License, Version 2.0` to list
@jen-huang jen-huang deleted the fix/indexed-fields-table-a11y branch December 16, 2019 22:55
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