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

[react-interactions] Add Portal propagation configuration #16889

Merged
merged 1 commit into from
Sep 25, 2019

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Sep 25, 2019

This adds Portal propagation configuration support to the new event system. By default responder targets will not propagate through React Portals, unless a targetPortalPropagation flag is set to true on the responder object.

@sizebot
Copy link

sizebot commented Sep 25, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 03df310

@necolas
Copy link
Contributor

necolas commented Sep 25, 2019

Oh I'd assigned this task to myself

@trueadm
Copy link
Contributor Author

trueadm commented Sep 25, 2019

@necolas Ah my bad, sorry about that! Thanks for the review :)

@trueadm trueadm merged commit d6d83d7 into facebook:master Sep 25, 2019
@trueadm trueadm deleted the portal-propagation branch September 25, 2019 14:56
@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Sep 25, 2019

:( I really don't like this approach since it's now no longer safe to convert to and from a portal and it still doesn't solve the problem when you don't use a portal so now you have to convert to a portal to solve it.

@trueadm
Copy link
Contributor Author

trueadm commented Sep 25, 2019

@sebmarkbage What approach do you think is better? Happy to revert and take an alternative approach?

@trueadm trueadm restored the portal-propagation branch September 25, 2019 15:01
@necolas
Copy link
Contributor

necolas commented Sep 25, 2019

There isn't a problem when not using a portal. I'd rather make it so individual portals can be configured to prevent event bubbling but you opposed that too in the existing thread about the issues people had when migrating to the stable createPortal API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants