feat(get-role): add noPresentational option #2549
Conversation
|
|
Add option to getRole to return null if 'presentation' or 'none' would be returned Closes issue #1792
codingLogan
changed the title
|
@WilcoFiers you'll need to review this as I consulted on it. |
straker
added
the
hacktoberfest
| if (noPresentational && ['presentation', 'none'].includes(role)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think this would be better done as an "else if" here https://github.com/dequelabs/axe-core/pull/2549/files#diff-1bf6cc2da8acf89f4e8559d8254862d7L143 instead of in a separate function. WDYT?
There was a problem hiding this comment.
Just to be sure I understand the suggestion clearly, I'll explain the reasoning behind my decisions.
I created the resolveRole(...) function from the understanding that the new option needs to handle whatever role gets returned from getRole(...). Since the original getRole(...) function had so many early returns, this was a great way for me to catch "whatever gets returned" as the role, and then apply the new option's behavior.
Let me know if that helps clarify why I did what I did. Thank you @WilcoFiers for your review
There was a problem hiding this comment.
@Loganras You are absolutely right. @straker reminded me that implicitRole can return presentational too. My suggestion doesn't work. Let's go with this.
|
@WilcoFiers , have you had a chance to review the comment I left earlier on your requested changes? I look forward to hearing from you with any suggestions you may have. |
| if (noPresentational && ['presentation', 'none'].includes(role)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
@Loganras You are absolutely right. @straker reminded me that implicitRole can return presentational too. My suggestion doesn't work. Let's go with this.
|
@Loganras Thank you for the contribution! Much appreciated. |
|
Thank you @WilcoFiers and @straker , it was my pleasure to contribute! |
Add option to getRole to return null if 'presentation' or 'none'
would be returned
Closes issue #1792
<< Describe the changes >>
Closes issue:
Reviewer checks
Required fields, to be filled out by PR reviewer(s)