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

[EuiPopover] Default to ownFocus #4551

Merged
merged 4 commits into from
Mar 4, 2021
Merged

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Feb 18, 2021

Summary

After the initial attempt was reverted this again sets ownFocus to default to true for EuiPopover.

Closes #4222

From the last time we tried upgrading Kibana with this update, we know the update will take some time/work so doing it now (at the very start of 7.13) would be good timing.

Checklist

  • [ ] Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox and with Kibana linked
  • [ ] Props have proper autodocs and playground toggles
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs 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

Co-authored-by: Elizabet Oliveira <[email protected]>
Co-authored-by: Caroline Horn <[email protected]>
Co-authored-by: Chandler Prall <[email protected]>
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2021

Can you remind me exactly what the problem was during the Kibana upgrade?

@miukimiu
Copy link
Contributor

@cchaos it's mentioned here #4311

@myasonik
Copy link
Contributor Author

So one of the two issues mentioned in #4311 is now closed. And the other, being around focus trapping, has changed so much in EUI since we last tried, that I think almost anything could happen (could be magically fixed, could be magically worse).

Either way, I think just trying the upgrade again, hopefully without much time pressure, is going to be the way forward.

CC @chandlerprall who was doing the upgrade - maybe he remembers more details

@cchaos
Copy link
Contributor

cchaos commented Feb 19, 2021

Thanks!

(could be magically fixed, could be magically worse).

My suggestion with this PR, then, is to ensure we're good Kibana-side with a local yarn link before we merge this PR. I'd like to avoid a possibly known situation of holding up other updates for this one.

@chandlerprall
Copy link
Contributor

The last-encountered hurdle in the previous Kibana upgrade attempt was the top nav dropdowns (e.g. "Alerts") wouldn't close for some reason. Echoing Caroline here, we'll want to test this PR heavily with a custom build in Kibana before merging.

@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor Author

Tested with a local version of EUI packaged into Kibana and everything seems to work! (Tested in FF, Chrome and Safari.)

@myasonik myasonik marked this pull request as ready for review February 22, 2021 22:56
@chandlerprall
Copy link
Contributor

Changes look good (and I pushed 3 more removals of ownFocus). I'll do a build & kibana test next, as well.

@kibanamachine
Copy link

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

@chandlerprall
Copy link
Contributor

chandlerprall commented Feb 25, 2021

The menu popovers close now, but seeing Discover's Inspect flyout remaining open. Unsure if this is a bug in kibana/master or introduced by the ownFocus change.

discover_inspect

@myasonik
Copy link
Contributor Author

myasonik commented Feb 25, 2021

I'm able to reproduce on master - opened elastic/kibana#92903
Screen Shot 2021-02-25 at 14 54 10

@chandlerprall
Copy link
Contributor

Great - kinda 😁

Last thing I want to test is this change with a full kibana functional test pass. Think I have a way to do that on Kibana's CI without making an EUI release.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Custom build of eui v31.7.0 + these changes passed all Kibana tests except snapshots 👍

@kibanamachine
Copy link

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

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.

[EuiPopover] should default to ownFocus
5 participants