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.

Sign up for GitHub

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

Jump to bottom

feat(standards): add superclassRole to ariaRoles #2600

Merged
merged 3 commits into from
Oct 30, 2020
Merged

feat(standards): add superclassRole to ariaRoles #2600

merged 3 commits into from
Oct 30, 2020

Conversation

redgardner
Copy link
Contributor

@redgardner redgardner commented Oct 27, 2020 •
edited by WilcoFiers
Loading

Added superclassRole attributes as an array of strings per WAI-ARIA 1.1 spec

Closes issue #2151

Added superclassRole to each aria-role per WAI-ARIA 1.1 spec

Closes issue:

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

  • Follows the commit message policy, appropriate for next version
  • Code is reviewed for security

@redg-indeed
feat(standards): add superclassRole to ariaRoles
86dfaf4 
Added superclassRole attributes as an array of strings per WAI-ARIA 1.1 spec

Closes issue 2151
@redgardner redgardner requested a review from a team as a code owner October 27, 2020 20:21
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@redgardner redgardner mentioned this pull request Oct 27, 2020
Closed
@straker
Copy link
Contributor

straker commented Oct 27, 2020

Thanks for the pr. Would you be able to add the superclassRole to the ARIA 1.2 roles as well? For example insertion

@redgardner
Copy link
Contributor Author

Sure thing! I'll make this update

@redg-indeed
feat(standards): add superclassRole to ariaRoles
94dcdb1 
    Added superclassRole attributes as an array of strings per WAI-ARIA 1.2 spec

    Closes issue 2151
@redgardner
Copy link
Contributor Author

Made the update. Is it correct to wait to squash these commits until merge to develop?

@straker
Copy link
Contributor

straker commented Oct 27, 2020 •
edited
Loading

Thanks! Yep, we'll squash the commit on our side so you don't have to worry about it.

@WilcoFiers WilcoFiers added the hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/ label Oct 28, 2020
WilcoFiers
WilcoFiers requested changes Oct 28, 2020
View reviewed changes
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

Couple minor edits. Should be good to go after that.

lib/standards/aria-roles.js Outdated
Comment on lines 182 to 183
// Note: spec difference
superclassRole: ['section'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note: spec difference
superclassRole: ['section'],
superclassRole: ['section'],
// Note: spec difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved this with the latest push and a few more areas where I had done this

lib/standards/aria-roles.js Outdated
},
none: {
type: 'structure'
type: 'structure',
superclassRole: ['section']
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the same as presentation.

Suggested change
superclassRole: ['section']
superclassRole: ['structure']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

lib/standards/aria-roles.js Outdated
},
roletype: {
type: 'abstract'
type: 'abstract',
superclassRole: ['structure', 'widget', 'window']
Copy link
Contributor

Choose a reason for hiding this comment

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

These are subclasses, not superclasses.

Suggested change
superclassRole: ['structure', 'widget', 'window']
superclassRole: []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Thanks for the catch

lib/standards/aria-roles.js Outdated
Comment on lines 667 to 668
// Note: spec difference
superclassRole: ['section'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Note: spec difference
superclassRole: ['section'],
superclassRole: ['section'],
// Note: spec difference

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed along with a few others I found

@redg-indeed
fix(standards) Minor changes from PR feedback
11feb81 
Fixed errors that were pointed out in the PR feedback

Closes issue 2151
@redgardner
Copy link
Contributor Author

Thanks I've addressed all the feedback and pushed up a new commit. I wasn't sure if that was the preferred way or accepting change recommendation.

@straker straker mentioned this pull request Oct 28, 2020
Merged
2 tasks
@WilcoFiers
Copy link
Contributor

@redgardner can you please click to sign the CLA. The link is https://cla-assistant.io/dequelabs/axe-core?pullRequest=2600

@redgardner
Copy link
Contributor Author

@WilcoFiers I have tried to sign the CLA in multiple browsers and windows. I've gotten confirmation that I've signed it, but I keep getting the pending CLA note.

I've tried hitting the recheck as well. Is there something else to try?

@WilcoFiers
Copy link
Contributor

@redgardner I'm not sure what's going on with the CLA site. We've been having some issues. I'll take your message as confirmation you agree, and will merge the pull request. Thank you very much for this contribution!

@WilcoFiers WilcoFiers merged commit a5e9ce0 into dequelabs:develop Oct 30, 2020
@redgardner
Copy link
Contributor Author

Thanks! Sorry for the hassle with the CLA stuff. I agree with the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WilcoFiers WilcoFiers WilcoFiers approved these changes

Assignees
No one assigned
Labels
hacktoberfest Help contribute during the month of October to participate https://hacktoberfest.digitalocean.com/
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@redgardner @CLAassistant @straker @WilcoFiers @redg-indeed