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

Unexpected side-effect when providing containerId #169

Closed
killthekitten opened this issue May 18, 2020 · 4 comments
Closed

Unexpected side-effect when providing containerId #169

killthekitten opened this issue May 18, 2020 · 4 comments

Comments

@killthekitten
Copy link

killthekitten commented May 18, 2020

Bug Report

Describe the Bug

The containerId parameter would create a new element with this id unless the container already exists. This is not a documented behaviour and I would expect an error in this case.

The docs just say this:

You can specify the container of portal you want by setting it as the id of the DOM element.

How to Reproduce

  1. Provide a containerId that doesn't exist on the page to the usePortal hook.
  2. Find a new element in the DOM with this containerId.

Expected Behavior

Error raised or nothing.

@wellyshen
Copy link
Owner

@killthekitten Sorry to confuse you. This hook will automatically creates the container element for you if it doesn't exist. I have refine the documentation in the latest version, thank you.

@killthekitten
Copy link
Author

killthekitten commented May 18, 2020

@wellyshen thanks for the update, looks better now. However, it feels like this behavior should be separated.

If the container is present most of the time, but not always, it might ruin the UI. Imagine a dynamic containerId, or just making an error in code. The user will see the "default" container created by the library, rather than a styled and tested container that was supposed to be there.

@wellyshen
Copy link
Owner

wellyshen commented May 18, 2020

@killthekitten Sorry I don’t fully understand your concerning. Currently, the library will creates the container element only when needed and removes the element once it’s not used. For the overall mechanism, which part can we improve?

@killthekitten
Copy link
Author

killthekitten commented May 18, 2020

@wellyshen I'm concerned that providing a custom containerId can render a new element when it wasn't intended.

I can see three scenarios of using this:

  • There's no element with containerId. I want to create it automatically, as a sibling to root.
  • There is an element with such an id, and I want to use it.
  • There's no element, but I expect it to be there. I don't want a new element to be created, and would prefer an error to be thrown rather then silently create a new element.

The library covers the first two cases, but not the third one.

I would suggest an option to enable/disable rendering the container automatically, i.e. createContainer. The naming isn't great tho.

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

No branches or pull requests

2 participants