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

Improving a11y of BottomBar and ControlBar #2861

Merged
merged 6 commits into from
Feb 25, 2020

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Feb 14, 2020

Summary

Closes #2855

Wasn't sure what the solution was going to look like so I played with it a bit and this is what I ended up with. Juggling a few things right now though so I'm hoping someone from the EUI team can help me push this PR over the finish line - all the jest tests seem to be broken for some reason?

Help!

Edit: something was wonked in my environment. Updating the snapshots now worked as expected.

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

@myasonik
Copy link
Contributor Author

jenkins test this

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.

Thanks for tackling this one. I just did a quick code review first.

src/components/bottom_bar/bottom_bar.tsx Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Outdated Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.tsx Outdated Show resolved Hide resolved
@myasonik
Copy link
Contributor Author

@thompsongl @chandlerprall Could y'all take a look at my tests and tell me what I'm doing wrong?

For some reason the jest tests now return a bunch of loading states though I don't see what I may have done to cause that in the PR...

@thompsongl
Copy link
Contributor

For some reason the jest tests now return a bunch of loading states though I don't see what I may have done to cause that in the PR...

Context menu tests with icons have been flaky for a bit: #2765

Were you consistently getting failures before updating the snapshot?

@JoshMock
Copy link
Member

jenkins retest this please

@myasonik
Copy link
Contributor Author

I'm pretty consistently getting the same results locally:

  • Running yarn test-unit -u will give me the expected mock
  • Whatever is run when I try to commit will give the still loading mock

That doesn't happen 100% of the time but pretty consistently now...

@thompsongl
Copy link
Contributor

@myasonik Go ahead and revert changes to any snapshots you deem unrelated to bottombar and controlbar. Use the -n flag if necessary to push and we'll see what CI does.

I'm working on mocking EuiIcon to prevent this kind of thing in the future.

@thompsongl
Copy link
Contributor

@myasonik Rebase or merge master and any snapshot flakiness should be resolved

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.

Thanks for the explanations!

@kibanamachine
Copy link

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

@myasonik myasonik merged commit 7c1f7dd into elastic:master Feb 25, 2020
@myasonik myasonik deleted the fix/a11y-bottom-bar branch February 25, 2020 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BottomBar can be difficult to navigate to
5 participants