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

added labels and ids to section form tabs #1798

Merged
merged 21 commits into from
Sep 7, 2018

Conversation

blackbaud-conorwright
Copy link
Contributor

Added missing aria labels and ids to section form tablist, tabs, and tabpanels

Resolves: #1375

@codecov-io
Copy link

codecov-io commented Jul 11, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@e6b7e54). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #1798   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?     425           
  Lines             ?    9122           
  Branches          ?    1349           
========================================
  Hits              ?    9122           
  Misses            ?       0           
  Partials          ?       0
Impacted Files Coverage Δ
...sectioned-form/sectioned-form-section.component.ts 100% <100%> (ø)
...modules/sectioned-form/sectioned-form.component.ts 100% <100%> (ø)
.../modules/vertical-tabset/vertical-tab.component.ts 100% <100%> (ø)
...dules/vertical-tabset/vertical-tabset.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 e6b7e54...c81f6ca. Read the comment docs.

@@ -1,14 +1,22 @@
<sky-vertical-tab
<sky-vertical-tab
role="tab"

Choose a reason for hiding this comment

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

Should we rethink how we're adding these attributes? In the sectioned form template, this works, but what if a consumer implements the vertical tab by itself? I'm assuming we'd want the same roles, etc. applied automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure. I definitely see role="tab" being there, but then we would probably want them to set their own controls. Hmm, I'll switch these to being on the content of a verticaly tab and take them as input on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Blackbaud-SteveBrush I've moved the attributes to be inputs on the vertical tab, including ID. Let me know what you think when you get the chance

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, hold off. This is failing for a lying accessibility issue 😕
I'm currently trying to fix it

@blackbaud-johnly
Copy link
Contributor

Created #1840 to document the new properties.

@blackbaud-conorwright
Copy link
Contributor Author

@Blackbaud-SteveBrush Alright, I think I finally got this one. I also have the roles and labelled by gets/sets set up to be inputs if we want that on section-forms, but I'm not so sure we want that since we control those elements and will always ensure their association (contained within the component template)

@Blackbaud-SteveBrush Blackbaud-SteveBrush self-assigned this Jul 30, 2018
@Blackbaud-TrevorBurch Blackbaud-TrevorBurch self-assigned this Sep 6, 2018
Copy link
Member

@Blackbaud-TrevorBurch Blackbaud-TrevorBurch left a comment

Choose a reason for hiding this comment

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

LGTM

@Blackbaud-AlexKingman Blackbaud-AlexKingman merged commit b8d820c into master Sep 7, 2018
@Blackbaud-AlexKingman Blackbaud-AlexKingman deleted the fix-sectioned-form-tabs-missing-labels branch September 7, 2018 12:58
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.

6 participants