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

[EuiFlyout] Fix usage of outsideClickCloses prop #4986

Merged
merged 8 commits into from
Aug 10, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 28, 2021

This prop was originally introduced to help with flyouts that aren't showing an overlay mask (ownFocus=true) to still allow any interaction on the page to close the flyout. The prop comment said it would also apply to the overlay mask, but it never did.

I've changed the default to just be undefined instead of false since it really all depends on the ownFocus option. And then set the onClose event on the overlay mask only if outsideClickCloses = false.

You can test it on the temporary change to the Without ownFocus example where I kept the ownFocus=true but just set outsideClickCloses=false.

Checklist

  • [ ] Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] 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

@cchaos
Copy link
Contributor Author

cchaos commented Jul 28, 2021

Also, if anyone wants to push a change to this PR to fix the props table for EuiFlyout, please go ahead! Ref: #4980

@kibanamachine
Copy link

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

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.

LGTM
Tested locally so I could look at various combinations of ownFocus and outsideClickCloses.

  • Changelog looks outdated; merge/rebase master before merging
  • Revert docs change

@cchaos
Copy link
Contributor Author

cchaos commented Aug 10, 2021

Waiting for the current release to finish, then I'll update the CL again

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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