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

Fix rootClose on unmounting components #117

Merged
merged 2 commits into from
Oct 1, 2016
Merged

Fix rootClose on unmounting components #117

merged 2 commits into from
Oct 1, 2016

Conversation

taion
Copy link
Member

@taion taion commented Sep 30, 2016

Most likely React unmounted the target in response to some other click.

@jquense This logic is not ideal but probably fine in practice? What do you think?

Most likely React unmounted the target in response to some other click.
Copy link
Member

@jquense jquense left a comment

Choose a reason for hiding this comment

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

I think it's fine. checking for detached nodes is part for the course for this sort of logic in my experience

@jquense
Copy link
Member

jquense commented Sep 30, 2016

par* why can't I edit anything

@taion
Copy link
Member Author

taion commented Oct 1, 2016

The alternative is to go back to the old approach, which ought to be safe to this kind of thing. wdyt? I think the "replace popover" scenario is rare enough that we don't have to rush out another release.

@taion
Copy link
Member Author

taion commented Oct 1, 2016

Actually, I don't think this works. Imagine the user clicks on a router link and navigates... this logic would still make the page transition. Is there a way to attach our document listener to fire before the React click handler?

@taion taion changed the title Don't rootClose on non-existent targets Fix rootClose on unmounting components Oct 1, 2016
@taion
Copy link
Member Author

taion commented Oct 1, 2016

@jquense You know DOM stuff a lot better than me... this is a reasonable fix?

@jquense
Copy link
Member

jquense commented Oct 1, 2016

capture seems fine...the thought is that if we fire before react, then there isn't any chance of the target being nonexistent due to an unmount caused by the event? I think its reasonable, I guess technically you could break it by using onClickCapture on a child component, but I think there are like 4 people that know that api exists in React, so probably not a problem :P.

@taion
Copy link
Member Author

taion commented Oct 1, 2016

What the heck is onClickCapture? What even is capture supposed to be other than an escape hatch for priority?

@jquense
Copy link
Member

jquense commented Oct 1, 2016

there aren't a ton of use cases for the capture flag, but generally its used for event delegation, e.g. you can mimic focusin event behavior by using focus with capture set to true.

The react event system will let you listen for the events with capture true if you add the "Capture" suffix to the event name(for events that support capture that is). I don't think anyone uses it but you can do like: <input onChangeCapture={handler}/> , At least you used to be able to.

@taion
Copy link
Member Author

taion commented Oct 1, 2016

I don't understand what any of that means. But, yeah, in practice this seems to mean that we fire before the target could potentially get unmounted by a normal React onClick or whatever handler.

@taion taion merged commit 3aa7e35 into master Oct 1, 2016
@taion taion deleted the rcw-doc branch October 1, 2016 22:44
@jquense
Copy link
Member

jquense commented Oct 1, 2016

🌈

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