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

EuiCard a11y #2521

Merged
merged 15 commits into from
Nov 14, 2019
Merged

EuiCard a11y #2521

merged 15 commits into from
Nov 14, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Nov 12, 2019

The main structural change

When providing an onClick or href property to EuiCard, the component used to display the whole contents within a button or anchor tag. This caused accessibility issues and invalid DOM structures since there would be block elements div inside of inline elements button.

Based on this article by Heydon Pickering, I went the route of the The redundant click event. Essentially, the main onClick or href is applied solely as a wrapper around the title. Then the full div element listens for it's own click event and will trigger the event wrapped around the title element.

Simplified rendered DOM:

<div class="euiCard ..."><!-- This div listens for an onClick -->
  <div class="euiCard__content">
    <span><!-- This element can be changed to any header level -->
     <button aria-describedby="descriptionID">Elastic Beats</button><!-- Accessible button with extra describedBy -->
    </span>
    <div class="euiText" id="descriptionID">
      <p>Example of a card's description. Stick to one or two sentences.</p>
    </div>
  </div>
</div>

The focus and hover states still work by showing the underline on the card's title.

Screen Recording 2019-11-12 at 06 09 PM

Cards with Beta badges

The beta badge label will at least assign its ID to the aria-describedby of the button similar to the code above that assigns the description's ID.

Selectable cards

The selectable button on EuiCard is now role="switch" to announce this behavior. Also, the whole card is clickable as well even if there's an extra link inside the body of the card.

Screen Recording 2019-11-13 at 09 33 AM

Additional change

Added the ability to supply children to EuiCard. The children will come after the still required title and description, but they won't align to the bottom of the card like the footer does.

Screen Shot 2019-11-13 at 09 25 53 AM

Fixes

I fixed #1543 where the horizontal layout with didn't quite layout correctly with icons.

Screen Shot 2019-11-13 at 09 29 56 AM

I also changed the title of the card to actually be one of our title sizes. I realized that it was in between our actual text scales and figured it's better to just go ahead and bump it so that it fits in our scales.

image

Fixed the contrast levels of disabled buttons

image

Todo later

Create a EuiCardList component that will take an array of card objects and spit them out as a list and list items.

Breaking change

I removed EuiCardGraphic. This was originally created to add a splash of color but were told by CS that it doesn't match our visuals so it was never officially used or documented.

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

@cchaos cchaos requested a review from miukimiu November 13, 2019 15:32
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.

Changes LGTM, pulled & tested locally. I tried giving the a vs. button + onClick target interaction but I think ultimately the complexity cost outweighs the gain, so I left it as is. Also thought about a useRef for storing the link's ref value but it isn't compatible with EuiCardSelect's buttonRef.

@cchaos
Copy link
Contributor Author

cchaos commented Nov 13, 2019

I added one more commit to fix some more a11y after running the page through Axe.

Screen Shot 2019-11-13 at 11 54 34 AM

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

💯

src/components/card/card.tsx Outdated Show resolved Hide resolved
src/components/card/_card.scss Show resolved Hide resolved
src/components/card/card.tsx Show resolved Hide resolved
@snide snide mentioned this pull request Nov 13, 2019
19 tasks
@@ -165,7 +171,6 @@

.euiCard.euiCard--horizontal {
.euiCard__content {
padding-top: $euiSizeS; // Aligns title and text a bit better and adds spacing in case of beta badge
text-align: left; /* 3 */
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just trying to figure out what the /* 1 */, /* 2 */ and /* 3 */ comments along this file mean. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes we apply certain styles across multiple selectors but for a single reason. So we put that reason up at the top of the document and number them. Then we use that number to correspond to the multiple styles we add. You can find these numbers at lines 7-12.

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

LGTM!

@cchaos cchaos merged commit 149e82a into elastic:master Nov 14, 2019
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.

EuiCard needs to better align the title in horizontal layout
4 participants