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

adds support to pass descriptionID to bind a button and a description… #1758

Closed
wants to merge 6 commits into from

Conversation

PhilippBaranovskiy
Copy link
Contributor

Fix: #1735

Summary

I've added passing descriptionId argument, also updated example as well as `changelog CHANGELOG.md

@PhilippBaranovskiy PhilippBaranovskiy marked this pull request as ready for review March 22, 2019 14:31
@PhilippBaranovskiy
Copy link
Contributor Author

@cchaos could you have a look at this? Please, let me know whether something could be better.

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.

I think this is a good, but possibly temporary solution, as there is more accessibility enhancements that can be made overall to this component. For that reason, I don't think it's necessary to use the callout so large in the documentation but allow the props docs to handle the description of the prop.

I'm also curious as to how we passed card accessibility on the EUI side but not in Kibana.

src-docs/src/views/card/card_example.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_footer.js Outdated Show resolved Hide resolved
src-docs/src/views/card/card_footer.js Outdated Show resolved Hide resolved
@@ -178,6 +172,11 @@ EuiCard.propTypes = {
*/
titleElement: PropTypes.oneOf(['h2', 'h3', 'h4', 'h5', 'h6', 'span']),
description: PropTypes.node.isRequired,
/**
* Allows you to bind a button from `footer` value to a description
* to make ScreenReader able to explain where that button refers.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can make this more succinct like and a little more generalized (it doesn't only need to be added to a footer button) like:

Passed down to description's <p> for binding via aria-labelledby on the card's call to action.

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.

Oh and also be sure to update the EuiCard's TS definitions in the card/index.d.ts

@PhilippBaranovskiy
Copy link
Contributor Author

@cchaos, I've got two questions:

  1. I do need to pass that this card accessibility change to Kibana (see (Accessibility) "Add" buttons provide no context on Sample Data page. kibana#28381)
  2. If it is supposed to be a temporary solution, then I have to decline that approach and perform something another, because I'm not sure whether it is a good idea to make any updates on Kibana relying on temporary implementations. Could you please clarify your thought on that?

@cchaos
Copy link
Contributor

cchaos commented Mar 22, 2019

As I mentioned in the issue #1735, there is a whole host of other accessibility improvements we can make to the EuiCard. We just hadn't made it a priority since it originally passed the accessibility tests #614. Therefore, this could possibly change when we do the accessibility improvement. I'll agree that maybe doing this now as a shim might cause breaking changes down the line, but I also don't want to block your implementation.

It's still possible to use the span with id in your implementation to create this effect. And I can put EuiCard accessibility higher on our roadmap so we can it done sooner.

@PhilippBaranovskiy
Copy link
Contributor Author

I'm closing this since @cchaos is going to update Card's accessibility by importing some ideas from the guidance, which in fact may cross out the proposed implementation.
This question is still under discussing.

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.

2 participants