Skip to content

[EuiPopover] Initial isOpen works with outside click#4461

Merged
thompsongl merged 8 commits intoelastic:masterfrom
thompsongl:4460-popover
Feb 3, 2021
Merged

[EuiPopover] Initial isOpen works with outside click#4461
thompsongl merged 8 commits intoelastic:masterfrom
thompsongl:4460-popover

Conversation

@thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Jan 29, 2021

Summary

Fixes #4460, in which initializing EuiPopover with isOpen=true would never set state.isOpenStable=true and therefore disable EuiOutsideClickDetector.

Previously, componentDidUpdate was the only place that state.isOpenStable was set to true. This moves the open popover logic outside of componentDidUpdate to a standalone function (onOpenPopover), which can now be called during componentDidMount.
The simpler way would just be set state.isOpenStable during componentDidMount, but that would bypass all timing functions. I could be persuaded that this simpler path is better, though.

Also, setting isOpening: true is no longer necessary in componentDidMount, but the Jest environment misses the update and causes tests and snapshots to break.

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

- [ ] 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

@thompsongl
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Test failures are due to axe. The open popover prevents axe from traversing the page correctly. Will be resolved when I revert the 'revert me' docs commit.

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.

...x-state...

LGTM! Tested the modified example, could not get into a broken state. This is awesome, and this bug also affected the datagrid expansion popover - would you mind removing usages of renderPopoverOpen from data_grid_cell.tsx as well?

@thompsongl
Copy link
Contributor Author

x-state

💯

would you mind removing usages of renderPopoverOpen from data_grid_cell.tsx as well?

Sure thing

@kibanamachine
Copy link

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

@thompsongl thompsongl merged commit b80fe23 into elastic:master Feb 3, 2021
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] closePopover isn't called on outside click when popover is in opened state from initial render

3 participants