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

Added aria-describedby to EuiFilePicker #2919

Merged
merged 9 commits into from
Feb 26, 2020
Merged

Conversation

anishagg17
Copy link
Contributor

@anishagg17 anishagg17 commented Feb 25, 2020

Summary

Fixes :#2917

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@anishagg17 anishagg17 requested review from cchaos, thompsongl, chandlerprall and myasonik and removed request for cchaos February 25, 2020 05:14
@myasonik
Copy link
Contributor

Hey Anish! Thanks for this PR!

I think I wasn't clear enough in the issue, sorry! I'm going to try to explain again. (Unfortunately, I think this will require a bit of a refactor of your work.)

So I think this issue can be broken down into two tasks:

  1. Use [html_id_generator](https://github.com/elastic/eui/blob/master/src/services/accessibility/html_id_generator.ts) to generate a unique id. (If you search EUI's codebase for that util, you should find several examples.) Then use that id as the id of the div.euiFilePicker__prompt and the value of aria-describedby of the <input />.

  2. If an id is passed into the component, instead of generating an id, use the passed in value with a modifier (such as {id}-filePicker__prompt). (Then using it the same way as we did in step 1.)

Hopefully that makes more sense and feel free to ask if anything isn't clear before jumping into the code!

@anishagg17
Copy link
Contributor Author

@myasonik all changes have been done

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround!

I left a few thoughts on what you've got here now.

src/components/form/file_picker/file_picker.tsx Outdated Show resolved Hide resolved
src/components/form/file_picker/file_picker.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/components/form/file_picker/file_picker.tsx Outdated Show resolved Hide resolved
@anishagg17
Copy link
Contributor Author

@myasonik Changes have been made

Copy link
Contributor

@myasonik myasonik 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 to me!

Please wait for an EUI team member to approve this PR before merging though (all the other reviewers are EUI team)

src/components/form/file_picker/file_picker.tsx Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2919/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

The CI failure is due to outdated test snapshots. This is expected, as markup changes like the ones in this PR need to be confirmed.

Run yarn test locally and you'll be prompted to review the snapshot expectations, and given a command to update outdated snapshots.

@anishagg17
Copy link
Contributor Author

@thompsongl Test Snapshot has been updated

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

Just a quick problem with the changelog

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2919/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Ah yes. The CI failure reminded me that htmlIdGenerator needs to be mocked in Jest tests, otherwise the id will change every run.

Add the following to src/components/form/file_picker/file_picker.tsx

// Mock the htmlIdGenerator to generate predictable ids for snapshot tests
jest.mock('../../../services/accessibility/html_id_generator', () => ({
  htmlIdGenerator: () => () => 'htmlId',
}));

And then you'll need to update the snapshots once again.

Hopefully we'll come up with a less manual solution than this soon

@anishagg17
Copy link
Contributor Author

@thompsongl changes Done 👍

@thompsongl
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_2919/

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.

5 participants