Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Added a11y attributes for text expand #1959

Merged
merged 6 commits into from
Sep 6, 2018

Conversation

Blackbaud-TrevorBurch
Copy link
Member

Resolves #1564

@codecov-io
Copy link

codecov-io commented Sep 4, 2018

Codecov Report

Merging #1959 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1959   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         424     424           
  Lines        8975    8980    +5     
  Branches     1327    1328    +1     
======================================
+ Hits         8975    8980    +5
Impacted Files Coverage Δ
src/modules/text-expand/text-expand.component.ts 100% <100%> (ø) ⬆️
...-expand-repeater/text-expand-repeater.component.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 577172b...8da257c. Read the comment docs.

@Blackbaud-MattGregg
Copy link
Contributor

@Blackbaud-TrevorBurch The buttons which expand/collapse the additional content look good. The ones which open modals have the same semantics though (i.e., aria-expands) which isn't correct. This attribute doesn't make sense for these as they don't expand/collapse. For the modals, you'll want to include the aria-haspopup="dialog" attribute instead and lose the aria-controls attribute.
https://www.w3.org/TR/wai-aria-1.1/#aria-haspopup

I also noticed that the text expand uses some variation of a special modal which isn't the exact modal component. It seems to be missing it's role="dialog" attribute which the regular modal has. It needs this added.

thx.

@Blackbaud-TrevorBurch
Copy link
Member Author

@Blackbaud-MattGregg I've added the aria-haspopup for the modal views. I also realized that I missed the repeater version of this component.

As far as the modal, I verified running locally and the modal does have the role=dialog. You don't see it in the code in the text expand module because it gets this by default via the sky-modal tag which the text expand's modal does use.

@Blackbaud-AlexKingman
Copy link
Contributor

@Blackbaud-TrevorBurch really nice test cleanup! It's McDonalds!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants