Add ability to render popup in different root element using portal

[dhruvparmar372]

Fixes #46

[dhruvparmar372]

@yjose thoughts on this?

[yjose]

Hi @dhruvparmar372, Sorry for my late.
As you mentioned on #46, the popup component will not work correctly inside a div with transform property because of the overlay has a fix position.
And as you know, one of the major benefits using reactjs-popup is that not inject HTML outside your app root.( your solution ).
I think we need to find A solution without using the portal.

[mucsi96]

Hi @yjose! Thanks for creating this awesome package! Can you explain why injecting HTML outside app root is a bad idea? As far as I understand because of nature how stacking contexts work in browser (they are not global but hierarchical) this is the only solution.

For example let's assume you want to have a popup for a text having ellipsis. For that you need to add overflow: hidden for the text. But with that you are cutting the popup as well if it is not moved out to the root stacking context. This is the main reason packages such as Popper.js exists.

Other example would be having a scrollable list of items. Let's assume we want to have a popup for every item in the list. As we add overflow-y: auto to make the list scrollable the browser will cut off our popup if it appears party outside the list.

Another example is that even if we give z-index: 9999 for our popup in element which is deeply tested within the DOM and the stacking context hierarchy we can easily make some element having z-index: 1 to appear above our popover if it's higher in the stacking context hierarchy. Which is most probably not what we what.

What do you think?

I am thinking about creating some similar package incorporating React's createPortal and Popper.js for placement. Would you accept my contribution into your package regarding adding Portal and Popper?

[dhruvparmar372]

@yjose

And as you know, one of the major benefits using reactjs-popup is that not inject HTML outside your app root.

The above solution makes rendering the popup to a portal optional, if you don't pass in popupRootID then the popup gets rendered in the app's root, thus not affecting any of the default behavior of the library.

An alternative approach might be to allow to wrap the app root itself with a HOC, something like this

function withPopupRoot(WrappedComponent) {
  return (props = {}) => (
    <React.Fragment>
      <div id="popupRoot"></div>
      <WrappedComponent {...props}/>
    </React.Fragment>
}

const Root = withPopupRoot(App)
ReactDOM.render(</Root>, document.getElementById('root'))

The HOC could be exposed as a function by the package and popupRootID prop can be used to render the popup in seperate root from the rest of the app. Though I'm not sure if popup's which are open by default would work well with this approach.

Whichever approach is taken, I feel it's necessary for the library to allow rendering a popup in an element which is not contained in the app's root element.

[kclark6595]

@dhruvparmar372 I am not seeing any mention of the popupRootID in any of the documentation. I am trying to get the modal to open on the very top level of my site. I have the Popup rendering on a table cell, and when the modal opens, it currently only opens within the confines of the table.

[dhruvparmar372]

@CodeHutDevelopment you can check the PR diff for the usage, https://github.com/yjose/reactjs-popup/pull/47/files#diff-66a12f065041e1923969613bdddd5d10R28

[kclark6595]

@dhruvparmar372 Ok, found that, so I added it to my code:

    <Popup
      trigger={<button className="button">{cellId}</button>}
      modal
      closeOnDocumentClick
      popupRootID="top"
    >
      <span> Record {cellId} </span>
    </Popup>

When I look at the react code in Developer, I am seeing the popupRootID in props and the modal is still rendering inside the DataTable instead of the tag (tag with id="top")

[avivshafir]

can these be released soon? what is missing for a merge and a new version release

[Akaren13]

Hi, I also need this function. Can you release it?

[vzaidman]

Hey!
I'd also like this feature to be released :D

[Akaren13]

Guys, can you fix it?

[flackeryy]

agreed this will be a very useful property

[gajus]

@dhruvparmar372 did you by any chance publish a fork of this package that includes this PR?

[gajus]

This is using global ReactDOM, which is unlikely to be present. Must import ReactDOM.

[gajus]

I have published this package as reactjs-popup-47 (with ReactDOM fix).

You can npm install reactjs-popup-47 and use popupRootID.

@yjose As a curtesy to contributors, it is nice to either close the PR and say you are not going to merge it, provide feedback what is required for PR to get merged, or merge the PR. It discourages community participation if PRs don't get merged and remain hanging literally for over a year.

[gajus]

@yjose Any chance of releasing this?

I've now used this in at least 3 projects.

[Nantris]

@yjose why not publish this? Why just close it without comment?

It's a shame to have to choose between a version that has custom portaling and a version that's still receiving updates. The built-in portal seems to mangle our layout.

I get the impression this library is unmaintained because of the handling of this issue, and indeed, the last merged PR is nearing a year old now.