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

fix(metadata): consistent use of 'must' and 'should' #2770

Merged
merged 1 commit into from
Jan 24, 2021

Conversation

WilcoFiers
Copy link
Contributor

Ensure that best practices never use the word "must" in its description / help text, and that other rules don't use the word "should".

Closes issue: #1491

@WilcoFiers WilcoFiers requested a review from a team as a code owner January 20, 2021 12:16
@stephenmathieson
Copy link
Member

Is this a "feature"? Seems like a docs or fix commit to me. 🤔

@WilcoFiers
Copy link
Contributor Author

@stephenmathieson Wouldn't say its docs... but yeah maybe it's more of a fix.

@WilcoFiers WilcoFiers changed the title feat(metadata): consistenct use of 'must' and 'should' fix(metadata): consistenct use of 'must' and 'should' Jan 20, 2021
Copy link
Member

@stephenmathieson stephenmathieson left a comment

Choose a reason for hiding this comment

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

Change LGTM, but @straker should probably take a look since this touches lib/rules.

@dylanb
Copy link
Contributor

dylanb commented Jan 21, 2021

@WilcoFiers this change does not seem right to me. Saying aria treeitem items should have an accessible name is not as accurate as saying aria treeitem items must have an accessible name

Under what circumstances are these items allowed to not have an accessible name?

@WilcoFiers
Copy link
Contributor Author

WilcoFiers commented Jan 21, 2021

@dylanb Yeah that one isn't too obvious. I ran this one by JC before we put it in. If a treeitem is interactive, it would need a name, but if it isn't, it's harder to argue that it does. ARIA says it does, which is why we put it as a best practice. But if you have a static treeitem, it's basically no different than a nested list. We don't require listitems to have an accessible name - ARIA doesn't either.

So that's why we landed on that one. I suppose we could've made it a WCAG rule and restrict it to focusable elements, but then again, we don't fail focusable listitems if they're empty either. This seemed like the most appropriate way to do it. It's maybe not as strict as it could be if we further refined it, but it also gets not false positives.

Treeitem didn't seem like the type of role to invest lots of time in. It's such an uncommon thing to have on a page. Bigger fish.

@dylanb
Copy link
Contributor

dylanb commented Jan 21, 2021

I am worried about all of them - that one was just an example.

I am concerned that these changes are not being made with usability in mind. We need to remember that 97%+ of the people reading these do not know the nuances that the JC would be concerned about.

I want to put the brakes on this and have a usability discussion about these first and foremost before we make any changes.

@jeeyyy jeeyyy changed the title fix(metadata): consistenct use of 'must' and 'should' fix(metadata): consistent use of 'must' and 'should' Jan 22, 2021
@dylanb
Copy link
Contributor

dylanb commented Jan 22, 2021

Discussed with @WilcoFiers : we are good to go on this

@WilcoFiers WilcoFiers merged commit 603b612 into develop Jan 24, 2021
@WilcoFiers WilcoFiers deleted the must-should-use branch January 24, 2021 12:50
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.

3 participants