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

feat(rule): aria-roledescription #1745

Merged
merged 4 commits into from Aug 20, 2019
Merged

feat(rule): aria-roledescription #1745

merged 4 commits into from Aug 20, 2019

Conversation

straker
Copy link
Contributor

@straker straker commented Aug 2, 2019

Aria-roledescription required too many nuances that couldn't adequately be captured in just an unsupported object:

  • required matching a list of supported roles
  • matcher objects don't allow matching of elements with implicit roles
  • required failing on any element without a role

As such, it was easier to turn it into its own rule that could capture these nuances. This is the same problem we'll encounter when trying to support role="text" as we'd want to make sure that it isn't used on an element with interactive children (something else that can't be captured in an unsupported object).

Closes issue: #1517

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

@straker straker requested a review from a team as a code owner August 2, 2019 18:05
Copy link
Contributor

@dylanb dylanb left a comment

Choose a reason for hiding this comment

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

What about the documentation?

Added a couple of comments that you should address

lib/rules/aria-roledescription.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-roledescription.js Outdated Show resolved Hide resolved
lib/rules/aria-roledescription.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-roledescription.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-roledescription.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-roledescription.json Outdated Show resolved Hide resolved
lib/checks/aria/aria-roledescription.js Outdated Show resolved Hide resolved
lib/rules/aria-roledescription.json Outdated Show resolved Hide resolved
lib/rules/aria-roledescription.json Outdated Show resolved Hide resolved
Copy link
Contributor

@jeeyyy jeeyyy left a comment

Choose a reason for hiding this comment

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

Some nitpick inline comments.

@@ -0,0 +1,14 @@
options = options || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, destructuring will be easier to read & will remove line 4.

const { supportedRoles = [] } = options

return true;
}

if (role && role !== 'presentation' && role !== 'none') {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, exit early again easier to read.

if(!role) {
  return false
}

if(!['presentation', 'none'].includes(role)) {
 return undefined
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Disagree. There is a single "exit early" condition here. Splitting that over two if-statements is confusing. Readability isn't just about having shorter statements.

return true;
}

if (role && role !== 'presentation' && role !== 'none') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Disagree. There is a single "exit early" condition here. Splitting that over two if-statements is confusing. Readability isn't just about having shorter statements.

@straker straker merged commit 16682fd into develop Aug 20, 2019
@straker straker deleted the roledescription branch August 20, 2019 15:17
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.

None yet

5 participants