Allows for the modal to be appended to another element rather than the document body. #183

jshthornton · 2016-06-02T06:18:41Z

Use the property parentSelector to either be a string (for query selecting), a function (so that a parent component can supply its node once it has been rendered), or a dom element.

The modal will detect if the parent element has changed and move it accordingly.

The only thing I am skeptical of is if React will remove the node from the parent element's children if other children are rendered (but it hasn't so far in testing).

claydiffrient · 2016-06-12T19:30:43Z

@jshthornton What's the use case for not appending to the body? I'd like some information about the problem before talking about the solution. In my mind, the body is an element always present when the modal is triggered to open. Also, the place the modal portal ends up being attached isn't all that important as it's just there to make sure that the modal exists outside of the content that gets hidden.

diasbruno · 2016-06-24T15:21:57Z

@claydiffrient maybe in a very specific container.

...splitting the view in two parts, the top one has a modal opened, the bottom div is free to perform any actions (?).

@jshthornton this case make sense?

dannybloe · 2016-08-01T13:25:02Z

I am very pro this enhancement. I was just looking if this is possible somehow and was about to just implement my own modal panel component until I saw this pull request.
Please merge 👍

We have a web app that allows you to open a help side-panel. However, as soon as you have a modal panel open, and this modal panel has a help button, then the side-panel is behind the overlay. So we want the overlay to be a child of a specific div and not the body.

rgdelato · 2016-08-09T19:09:19Z

I also have a use case for this and would greatly appreciate having an official, non-hacky way to append the modal to an element other than document.body.

Our use case is that we'd like to re-style the modal content as body content on mobile devices, and so the modal needs to live as a sibling to the main content container.

diasbruno · 2016-08-09T20:15:01Z

I tested the branch, it's working and didn't break the tests. some observations...

1 - the ariaAppHider is not updated to use the parentSelector. maybe add a test for this?

2 - the overlay has fixed position and it'll have the same behavior if we change where the modal element should be appended, so you'll need to pass the new overlay style...should the modal update the position to absolute when parentSelector is defined?

3 - still needs testing when more than one modal is in the page.

claydiffrient · 2016-08-10T05:26:00Z

lib/components/Modal.js

@@ -10,6 +10,18 @@ var Assign = require('lodash.assign');
 var SafeHTMLElement = ExecutionEnvironment.canUseDOM ? window.HTMLElement : {};
 var AppElement = ExecutionEnvironment.canUseDOM ? document.body : {appendChild: function() {}};

+function getParentElement(parentSelector) {


I'm personally not so much of a fan of handling all these options here. Lately I've felt like maybe just going the function route is the best. That way the prop in all cases is just a function that returns a DOM node. Mixed examples don't have to be created and you don't have any issues with querySelector not returning what you expect it to, etc. Push the selecting of an element all out to userland rather than dealing with it in internal to the library.

msuperina · 2016-08-23T08:57:13Z

I don't understand why there is even the question about why one would want to append the modal somewhere else from the body. As a maintainer of my UI I want to organize my DOM in a way that suits me, I don't want to be constrained about the behaviour of a library. This body appending is a valid reason that prevents me to use this module that I would otherwise consider very helpful.

msuperina · 2016-08-23T12:20:59Z

lib/components/Modal.js

@@ -10,6 +10,18 @@ var Assign = require('lodash.assign');
 var SafeHTMLElement = ExecutionEnvironment.canUseDOM ? window.HTMLElement : {};
 var AppElement = ExecutionEnvironment.canUseDOM ? document.body : {appendChild: function() {}};


Why not going to the bottom of it and also modifying the way AppElement is resolved ? It would be great if document.body was never touched if a user wished so.

jshthornton · 2016-08-23T12:23:33Z

Sorry this has taken me so long to get around to commenting on.
So the reason why I specifically needed this was because I had a drag and drop component in my app which allows for any element below it to be dragged into. In my case it was very high up in the component tree to allow for the entire window to be draggable. However, when I threw this module in it rendered itself outside of the tree and was not partial to the dragging.

claydiffrient · 2016-08-23T12:45:44Z

@jshthornton I'm willing to merge this in once the conflicts are resolved and the comments that have been left have been addressed. As @diasbruno mentioned, this won't actually change anything about how the overlay works since it is fixed position. Everything will behave identically at this point whether you give it document.body or some_random_div other than the ModalPortal element itself will be in another location in the DOM which seems like it isn't really what you want.

jshthornton · 2016-08-23T12:49:31Z

I should be able to find some time during this week to get all of the
conflicts sorted and documented.
Anyone else in this thread have any opinions / proposals for changes of
implementation before I do?

On Tue, 23 Aug 2016 at 13:45 Clay Diffrient notifications@github.com
wrote:

@jshthornton https://github.com/jshthornton I'm willing to merge this
in once the conflicts are resolved and the comments that have been left
have been addressed. As @diasbruno https://github.com/diasbruno
mentioned, this won't actually change anything about how the overlay works
since it is fixed position. Everything will behave identically at this
point whether you give it document.body or some_random_div other than the
ModalPortal element itself will be in another location in the DOM which
seems like it isn't really what you want.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#183 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACrzGjEQwCWy82BijCjMxZup6yQEE_G7ks5qiut6gaJpZM4IsPNL
.

claydiffrient · 2016-09-14T13:35:32Z

@jshthornton I can't think of any other things about this. Will you have time to get to it soon? I've got a few commits that are on master now that I'd like to get cut into a release. I'm thinking a 2.0.0 release is forthcoming. I think I'd like to get this all squared away before doing that though.

cc/ @diasbruno

NOTE: react-modal removed the ability to select where the modal div is inserted into in version 1.3.0. This is because aria-hidden (i.e. "ignore this" for screenreaders) is added to the rest of the app, but now that the modal is also included in the app, it gets hidden from screenreaders as well. There's currently a PR open (reactjs/react-modal#183) that tries to address this, so we might see the return of this feature at some point. We should upgrade then.

# Conflicts: # lib/components/Modal.js

thien-do · 2016-09-28T19:47:46Z

Hi guys, I'm really interesting in this. It seems @jshthornton is busy, while I have some free time, so I hope I can help. I created a PR (jshthornton#1) to @jshthornton's one, so he can simply merge it when I finish. My PR:

Merge from upstream and resolve conflicts. (AFAIK, only one: e5b0181)
Allow parent selector to be function only, as suggested by @claydiffrient . However, I don't know should we check and/or throw error here
I'm very sorry but I don't understand @msuperina comment so I can't do anything with that. Can someone explain it to me, then I can work on that
I'll also add documentation to this PR soon

Merge with upstream master

update README

thien-do · 2016-09-30T16:09:56Z

@jshthornton @claydiffrient I have added the documentation for using custom node for the modal, please take a look (because I'm afraid that I'm not good at writing docs)

One last thing, should I apply @msuperina comment? If yes, can anyone explain to me.. because I don't understand his comment

thien-do · 2016-10-05T16:18:02Z

Hi @claydiffrient , do you have sometime to review this PR please?

thien-do · 2016-10-27T09:27:43Z

Hi @claydiffrient , do you intent to merge this PR? If so, I will resolve the conflict issues again.

claydiffrient · 2016-10-27T14:08:55Z

@dvkndn Yes. I do intend to merge it, but I've had very limited time to spend working on this lately :(. I will try and review it this weekend. If you want to solve the conflicts before then that would be great.

# Conflicts: # lib/components/Modal.js

thien-do · 2016-10-27T18:42:49Z

Thank you @claydiffrient . I have created a PR into @jshthornton PR jshthornton#3

After @jshthornton merge my PR, you can merge his

Pull upstream

thien-do · 2016-10-28T16:28:45Z

@claydiffrient please take a look when you're free. @jshthornton have merged my PR, so this PR is no longer has conflict

claydiffrient

Unfortunately when I ran npm test in the repo almost all the tests fail. I wish that I had access to enough of the repo to make Travis run that for you.

The problem as far as I can tell deals with the fact that currently the default parentSelector is document.body whereas it really expects it to be a function so it's failing with parentSelctor is not a function

claydiffrient · 2016-10-31T02:17:45Z

lib/components/Modal.js

@@ -37,6 +41,11 @@ var Modal = React.createClass({
    closeTimeoutMS: React.PropTypes.number,
    ariaHideApp: React.PropTypes.bool,
    shouldCloseOnOverlayClick: React.PropTypes.bool,
+    parentSelector: React.PropTypes.oneOfType([


This should only be React.PropTypes.func now correct?

claydiffrient · 2016-10-31T02:20:27Z

lib/components/Modal.js

@@ -47,24 +56,36 @@ var Modal = React.createClass({
      portalClassName: 'ReactModalPortal',
      ariaHideApp: true,
      closeTimeoutMS: 0,
-      shouldCloseOnOverlayClick: true
+      shouldCloseOnOverlayClick: true,
+      parentSelector: document.body


I think maybe specifying a function here would be best something like () => document.body would probably do.

thien-do · 2016-10-31T06:41:02Z

Oh I'm sorry about that. I just merged the PR with upstream. Now I will check the issue

…iew)

blainekasten · 2016-11-07T17:50:18Z

Another use-case for this is that it fails when a different element goes into fullscreen. For example, we use this lib to provide a modal over a video. But if you go into fullscreen on the video (which does not target document.body for specific reasons), the modal is now longer visible ever.

Would love to see this get merged.

# Conflicts: # README.md

Merge upstream and fix parent selector

thien-do · 2016-11-07T18:31:18Z

@claydiffrient I have fixed the issue, now the tests are passed, thank you for pointing out :D
it seems @jshthornton also merged it so you can review now

thien-do · 2016-12-06T12:20:44Z

Hi @claydiffrient is there any update?

claydiffrient · 2016-12-06T15:03:51Z

@dvkndn I'm sorry. Life has been super busy and I haven't had a moment to look at this. I'll re-review it quickly and see about getting it merged.

claydiffrient · 2016-12-06T15:10:06Z

Released in 1.6.0

jshthornton added 2 commits June 2, 2016 13:57

The ability to specify the parent via a selector

584de6e

Using object to denote dom element

9ada859

claydiffrient reviewed Aug 10, 2016
View reviewed changes

msuperina reviewed Aug 23, 2016
View reviewed changes

claydiffrient mentioned this pull request Sep 7, 2016

setAppElement() not working as documented #221

Closed

claydiffrient mentioned this pull request Sep 22, 2016

When will be next release? #229

Closed

Thien added 2 commits September 29, 2016 02:30

Merge remote-tracking branch 'upstream/master'

b3659e1

# Conflicts: # lib/components/Modal.js

allow only function as parentSelector

0e426dd

Merge pull request #1 from dvkndn/master

434ca7a

Merge with upstream master

claydiffrient mentioned this pull request Sep 30, 2016

Close modal when mouseDown and MouseUp happen only on the overlay #217

Merged

5 tasks

Thien and others added 2 commits September 30, 2016 23:05

update README

b8717b2

Merge pull request #2 from dvkndn/master

c0efb4b

update README

Merge branch 'master' of https://github.com/reactjs/react-modal

11c6346

# Conflicts: # lib/components/Modal.js

Merge pull request #3 from dvkndn/master

66a42fe

Pull upstream

claydiffrient suggested changes Oct 31, 2016

View reviewed changes

kozborn added a commit to evidenceprime/react-modal that referenced this pull request Nov 7, 2016

Changes requested by claydiffrient from react-modal. reactjs#183 (rev…

6e2d121

…iew)

Thien and others added 3 commits November 8, 2016 01:23

Fix parent selector type and default value

f1934bc

Merge branch 'master' of https://github.com/reactjs/react-modal

5170716

# Conflicts: # README.md

Merge pull request #4 from dvkndn/master

890ed6f

Merge upstream and fix parent selector

claydiffrient approved these changes Dec 6, 2016

View reviewed changes

claydiffrient merged commit de14816 into reactjs:master Dec 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows for the modal to be appended to another element rather than the document body. #183

Allows for the modal to be appended to another element rather than the document body. #183

jshthornton commented Jun 2, 2016 •

edited

Loading

claydiffrient commented Jun 12, 2016

diasbruno commented Jun 24, 2016

dannybloe commented Aug 1, 2016

rgdelato commented Aug 9, 2016 •

edited

Loading

diasbruno commented Aug 9, 2016

claydiffrient Aug 10, 2016

msuperina commented Aug 23, 2016

msuperina Aug 23, 2016

jshthornton commented Aug 23, 2016

claydiffrient commented Aug 23, 2016

jshthornton commented Aug 23, 2016

claydiffrient commented Sep 14, 2016

thien-do commented Sep 28, 2016

thien-do commented Sep 30, 2016

thien-do commented Oct 5, 2016

thien-do commented Oct 27, 2016

claydiffrient commented Oct 27, 2016

thien-do commented Oct 27, 2016

thien-do commented Oct 28, 2016

claydiffrient left a comment

claydiffrient Oct 31, 2016

claydiffrient Oct 31, 2016

thien-do commented Oct 31, 2016

blainekasten commented Nov 7, 2016

thien-do commented Nov 7, 2016

thien-do commented Dec 6, 2016

claydiffrient commented Dec 6, 2016

claydiffrient commented Dec 6, 2016

		@@ -10,6 +10,18 @@ var Assign = require('lodash.assign');
		var SafeHTMLElement = ExecutionEnvironment.canUseDOM ? window.HTMLElement : {};
		var AppElement = ExecutionEnvironment.canUseDOM ? document.body : {appendChild: function() {}};

Allows for the modal to be appended to another element rather than the document body. #183

Allows for the modal to be appended to another element rather than the document body. #183

Conversation

jshthornton commented Jun 2, 2016 • edited Loading

claydiffrient commented Jun 12, 2016

diasbruno commented Jun 24, 2016

dannybloe commented Aug 1, 2016

rgdelato commented Aug 9, 2016 • edited Loading

diasbruno commented Aug 9, 2016

claydiffrient Aug 10, 2016

Choose a reason for hiding this comment

msuperina commented Aug 23, 2016

msuperina Aug 23, 2016

Choose a reason for hiding this comment

jshthornton commented Aug 23, 2016

claydiffrient commented Aug 23, 2016

jshthornton commented Aug 23, 2016

claydiffrient commented Sep 14, 2016

thien-do commented Sep 28, 2016

thien-do commented Sep 30, 2016

thien-do commented Oct 5, 2016

thien-do commented Oct 27, 2016

claydiffrient commented Oct 27, 2016

thien-do commented Oct 27, 2016

thien-do commented Oct 28, 2016

claydiffrient left a comment

Choose a reason for hiding this comment

claydiffrient Oct 31, 2016

Choose a reason for hiding this comment

claydiffrient Oct 31, 2016

Choose a reason for hiding this comment

thien-do commented Oct 31, 2016

blainekasten commented Nov 7, 2016

thien-do commented Nov 7, 2016

thien-do commented Dec 6, 2016

claydiffrient commented Dec 6, 2016

claydiffrient commented Dec 6, 2016

jshthornton commented Jun 2, 2016 •

edited

Loading

rgdelato commented Aug 9, 2016 •

edited

Loading