Skip to content

Fix Storybook accessibility violations for the file input (#6124) #6417

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
Merged
merged 2 commits into from
Oct 13, 2022
Merged

Fix Storybook accessibility violations for the file input (#6124) #6417

merged 2 commits into from
Oct 13, 2022

Conversation

miqh
Copy link
Contributor

@miqh miqh commented Oct 12, 2022

Hello there,

This should fix up the accessibility violations reported across some of the FileInput stories.

It seems like using a label element, explicit or implicit, appears to be the preferred way to work around such violations. In the interest of preserving the existing story visuals, I opted for aria-label. If the former is more desirable, I can update my changes.

What does this PR do?

Resolve accessibility violations for FileInput stories.

Where should the reviewer start?

Changes are confined to the file input stories reporting violations.

What testing has been done on this PR?

Confirmed the violations no longer appear in the Storybook.

How should this be manually tested?

As above.

Do Jest tests follow these best practices?

N/A.

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

None.

What are the relevant issues?

Refer to #6124.

Screenshots (if appropriate)

N/A.

Do the grommet docs need to be updated?

Don't think so.

Should this PR be mentioned in the release notes?

Up to maintainers to decide.

Is this change backwards compatible or is it a breaking change?

Should have no impact on consumers of the library.

jcfilben
jcfilben reviewed Oct 12, 2022
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

This looks good, can we also add an aria-label to the FileInput/Custom/CustomThemed story as well? I know it was left out from the issue description checklist but just happened to notice it while I was reviewing.

@miqh
Copy link
Contributor Author

miqh commented Oct 12, 2022

@jcfilben, can do.

Until now that you've pointed out, I didn't even notice the theme switcher option in Storybook. My bad!

A quick look after, it seems like there are additional colour contrast violations in the "hpe" theme. What's your preferred approach with resolving these for now given that any colour tweaks will affect component consumers?

@jcfilben
Copy link
Collaborator

Good catch on the hpe theme! For now let's just leave the hpe ones as they are. I will bring the issue up with other members of the grommet team and we can decide how to move forward on the hpe theme issue, but don't worry about it for now in this pull request.

@miqh
Copy link
Contributor Author

miqh commented Oct 13, 2022

@jcfilben, got it.

Beyond FileInput, there are other components which exhibit that colour contrast violation (though I'm sure you're probably already aware). If you reach a consensus on what to do about that, then those violations can hopefully be dealt with in a uniform manner as well.

jcfilben
jcfilben approved these changes Oct 13, 2022
View reviewed changes
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good!

@ericsoderberghp ericsoderberghp merged commit aad44bf into grommet:master Oct 13, 2022
@jcfilben jcfilben mentioned this pull request Oct 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcfilben jcfilben jcfilben approved these changes

+1 more reviewer

@ericsoderberghp ericsoderberghp ericsoderberghp approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@miqh @jcfilben @ericsoderberghp