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

[EuiWrappingPopover] Do not re-mount button on componentWillUnmount if button has been unmounted from DOM #1114

Merged
merged 2 commits into from
Aug 13, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Aug 13, 2018

There is a bug in Kibana where the anchor element gets re-attached on componentWillUnmount even if the anchor element has been removed from the DOM.

To see the bug in action, first open a dashboard in edit mode and click the Options top nav button to open the Options popover.

screen shot 2018-08-13 at 1 23 54 pm

Then click the Cancel top nav button (while the Options popover is still open). Dashboard will remove the Options top nav from the menu bar. but then componentWillUnmount will add it back.

screen shot 2018-08-13 at 1 24 32 pm

It should look like this.

screen shot 2018-08-13 at 1 24 24 pm

This PR only re-mounts the button component when it still has a parent (is attached to the DOM). I have tested this in Kibana and this fixes the problem shown above

@nreese nreese requested a review from chandlerprall August 13, 2018 19:28
@nreese nreese changed the title Do not re-mount button on componentWillUnmount if button has been unmounted from DOM [EuiWrappingPopover] Do not re-mount button on componentWillUnmount if button has been unmounted from DOM Aug 13, 2018
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.

Change LGTM; I pulled and re-ran the docs for this component.

@nreese nreese merged commit 8bfffc5 into elastic:master Aug 13, 2018
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.

2 participants