feat(Portal): Improve rendering #666

jeffcarbs · 2016-10-12T18:17:01Z

Fixes bug in rendering composite react components as portal children
Add closeOnBackgroundClick option to enable multiple portals on top of each other without closing all on click outside.
Update modal to make use of this option (fixes feat(Modal): support opening/closing multiple modals #636)
Fix portal wrapper having undefined as a class name if none was passed.

This fixes a few issues uncovered when using the Portal directly. The biggest was the inability to have a non-DOM react component as the portal child. The fix was:

-    this.portal = ReactDOM.unstable_renderSubtreeIntoContainer(
+    ReactDOM.unstable_renderSubtreeIntoContainer(
       this,
       Children.only(children),
       this.node
     )

+    this.portal = this.node.firstElementChild

The issue was that unstable_renderSubtreeIntoContainer will return the DOM node if the element to render was a ReactDOM element (e.g. div) and null if the element was a regular react components. This meant for components, subsequent usage of this.portal would fail.

codecov-io · 2016-10-13T04:37:26Z

Current coverage is 100% (diff: 100%)

Merging #666 into master will not change coverage

@@           master   #666   diff @@
====================================
  Files         132    132          
  Lines        2102   2110     +8   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         2102   2110     +8   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update 359392e...bcd490d

jeffcarbs · 2016-10-13T04:47:28Z

@levithomason - this is ready for review 👍

levithomason · 2016-10-13T04:55:31Z

src/addons/Portal/Portal.js

      this,
      Children.only(children),
      this.node
    )

+    this.portal = this.node.firstElementChild


Any reason this shouldn't be Children.only(children)? If so, it could be assigned to a const at the top of the function and used here and in line 329.

I logged both so you can see:

console.log('children', Children.only(children)) console.log('firstElementChild', this.node.firstElementChild)

LOG: 'children', Object{$$typeof: 60103, type: 'p', key: null, ref: null, props: Object{children: 'Hi'}, _owner: null, _store: Object{}} LOG: 'firstElementChild', <p data-reactroot="">Hi</p> ✔ maintains ref to DOM node with host element LOG: 'children', Object{$$typeof: 60103, type: function Component(props) { ... }, key: null, ref: null, props: Object{}, _owner: null, _store: Object{}} LOG: 'firstElementChild', <p data-reactroot="">Hi</p> ✔ maintains ref to DOM node with React component

In both cases we need that actual DOM node, not a reference to the component or element.

levithomason · 2016-10-13T05:11:37Z

src/modules/Modal/Modal.js

@@ -188,6 +188,8 @@ class Modal extends Component {

    return (
      <Portal
+        closeOnBackgroundClick
+        closeOnDocumentClick={false}


Suggest setting defaultProps for this so there is visibility in the docs regarding the behavior. Though, IMO it would seem the default would be to close on escape and on document click. Feedback?

Currently we take unhandled Modal props and pass them to Portal if it handles them. Adding these to defaultProps would mean that they wouldn't get passed to the Portal, since getUnhandledProps includes defaultProps keys in determining what a component handles. We can't just take all props and filter ones that Portal handles because things like className would be double-applied.

Is there any reason why getUnhandledProps can't just use _.keys(Component.propTypes) only? Seems like if there are any autoControlled or defaultProps that a component consumes that aren't defined in the propTypes that's a bigger issue.

levithomason · 2016-10-13T06:24:18Z

src/addons/Portal/Portal.js

+     * - DocumentClick - any click not within the portal
+     * - BackgroundClick - a click not within the portal but within the portal's wrapper
+     */
+    closeOnBackgroundClick: customPropTypes.every([


If I'm reading corretly, the "Background" is the Dimmer when using a Modal with a Dimmer. Though, this prop name wouldn't apply to say, a Popup, or some other component. If so, what about using something like closeOnRootNodeClick instead?

levithomason · 2016-10-13T06:28:56Z

A few other issues that did not have a diff to comment on:

The Dimmer Variations example for None does not close on click outside.
The Close Config example for No Close On Click Outside closes on click outside. We should probably update this to show closeOnBackgroundClick with a Dimmer and closeOnDocumentClickwith no Dimmer to illustrate the differences.
Could we also add a Multiple Modal example?
Depending on the defaults we settle with, this could be a BREAKING CHANGE: ... PR. If the Modal no longer behaves the same with closeOnDocumentClick (not closing on Dimmer click).

- Fixes bug in rendering composite react components as portal children - Add closeOnBackgroundClick option to enable multiple portals on top of each other without closing all on click outside. - Update modal to make use of this option.

jeffcarbs · 2016-10-13T16:24:28Z

The Dimmer Variations example for None does not close on click outside.

This is expected behavior (works like SUI core). I added a notification popup on that button to explain what's happening. Essentially Modals can close on dimmer click. If there's no dimmer there's nothing to click on. You can manually specify closeOnDocumentClick which means any outside click.

The Close Config example for No Close On Click Outside closes on click outside. We should probably update this to show closeOnBackgroundClick with a Dimmer and closeOnDocumentClickwith no Dimmer to illustrate the differences.

Fixed

Could we also add a Multiple Modal example?

Added

Depending on the defaults we settle with, this could be a BREAKING CHANGE: ... PR. If the Modal no longer behaves the same with closeOnDocumentClick (not closing on Dimmer click).

No defaults changed. Closing on dimmer click still works as expected. When dimmer is set to false that essentially means "no dimmer" and since Modals can close on dimmer click, if there's no dimmer there's nothing to click on.

jeffcarbs · 2016-10-13T16:26:23Z

@levithomason - I think this should be g2g 👍 I also wound up renaming the modal examples to be inline with the new naming conventions.

levithomason · 2016-10-14T22:18:41Z

Cool, we can merge this, but it doesn't actually fix #636 since Escape and a click on the Dimmer still closes all modals. It should only close the top modal, then escape again would close the next modal. I'll update the description before merging.

EDIT

Hm, it looks like the intent was to allow closing one modal at a time. I'll wait for your response on what you'd like to do here. Pressing Escape/clicking the Dimmer does close both Modals. Here, I open both Modals then click the Dimmer. Second, I repeat but this time hit Escape. Notice both Modals close at the same time for both cases:

jeffcarbs · 2016-10-15T21:36:58Z

@levithomason - The example currently functions exactly like SUI core (see http://semantic-ui.com/modules/modal.html#/examples). Clicking the dimmer or pressing escape closes all open modals.

IMHO, this opens the possibility of multiple modals being open at once, and clicking outside closing only one. The example sets dimmer to false for the second modal, but if you left the default each modal would have its own dimmer so a click on the top modal's dimmer would only close that one. Escape is handled at the document level so as it stands it would always close all.

That being said, I'd be OK merging this but not closing #636. To truly handle that, I had an idea: Create a Modal.Group that would take an array of Modals (either via prop or children) and show them in succession within the same Portal. We could implement the allowMultiple prop like SUI core:

allowMultiple: false (default) would show each modal one at a time
allowMultiple: true would stack each new modal on top of one another

This is basically just a stateful wrapper around multiple modals. Given the current Modal implementation, a user could create something like this themselves so I don't think it should be something we need to do for v1. It's also not really part of SUI. In those examples they just daisy-chain the single modals together manually. This would really just be a SUI-react feature.

Sorry if that was a little scattered. The summary is that I think we should merge this since it fixes bugs (the this.portal bug is a blocker within in my company's app) and adds more functionality and complete any future work on separate PRs.

jeffcarbs · 2016-10-17T13:45:39Z

Bump 😄

levithomason · 2016-10-17T18:08:22Z

Thanks for the clarification, this makes sense.

levithomason · 2016-10-17T18:26:12Z

Released in semantic-ui-react@0.56.2

jeffcarbs force-pushed the feature/portal-enhancements branch from ff9b19c to 8115e75 Compare October 13, 2016 04:37

jeffcarbs added bug enhancement ready for review labels Oct 13, 2016

levithomason requested changes Oct 13, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Oct 13, 2016

jeffcarbs added 3 commits October 13, 2016 12:21

feat(Portal): Improve rendering

6c49d86

- Fixes bug in rendering composite react components as portal children - Add closeOnBackgroundClick option to enable multiple portals on top of each other without closing all on click outside. - Update modal to make use of this option.

feat(Portal): Add prepend option

de4aae3

Update modal examples, update portal props

bcd490d

jeffcarbs force-pushed the feature/portal-enhancements branch from 8115e75 to bcd490d Compare October 13, 2016 16:21

jeffcarbs added ready for review and removed needs author feedback labels Oct 13, 2016

jeffcarbs mentioned this pull request Oct 14, 2016

feat(Dimmer): Add component #447

Merged

levithomason added needs author feedback and removed ready for review labels Oct 14, 2016

jeffcarbs removed the needs author feedback label Oct 15, 2016

levithomason added the ready for review label Oct 16, 2016

levithomason approved these changes Oct 17, 2016

View reviewed changes

levithomason merged commit 12a6a36 into master Oct 17, 2016

levithomason deleted the feature/portal-enhancements branch October 17, 2016 18:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Portal): Improve rendering #666

feat(Portal): Improve rendering #666

jeffcarbs commented Oct 12, 2016 •

edited

Loading

codecov-io commented Oct 13, 2016 •

edited

Loading

jeffcarbs commented Oct 13, 2016

levithomason Oct 13, 2016

jeffcarbs Oct 13, 2016 •

edited

Loading

levithomason Oct 13, 2016

jeffcarbs Oct 13, 2016 •

edited

Loading

levithomason Oct 13, 2016

levithomason commented Oct 13, 2016 •

edited

Loading

jeffcarbs commented Oct 13, 2016

jeffcarbs commented Oct 13, 2016

levithomason commented Oct 14, 2016 •

edited

Loading

jeffcarbs commented Oct 15, 2016 •

edited

Loading

jeffcarbs commented Oct 17, 2016

levithomason commented Oct 17, 2016

levithomason commented Oct 17, 2016

feat(Portal): Improve rendering #666

feat(Portal): Improve rendering #666

Conversation

jeffcarbs commented Oct 12, 2016 • edited Loading

codecov-io commented Oct 13, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

jeffcarbs commented Oct 13, 2016

levithomason Oct 13, 2016

Choose a reason for hiding this comment

jeffcarbs Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 13, 2016

Choose a reason for hiding this comment

jeffcarbs Oct 13, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 13, 2016

Choose a reason for hiding this comment

levithomason commented Oct 13, 2016 • edited Loading

jeffcarbs commented Oct 13, 2016

jeffcarbs commented Oct 13, 2016

levithomason commented Oct 14, 2016 • edited Loading

jeffcarbs commented Oct 15, 2016 • edited Loading

jeffcarbs commented Oct 17, 2016

levithomason commented Oct 17, 2016

levithomason commented Oct 17, 2016

jeffcarbs commented Oct 12, 2016 •

edited

Loading

codecov-io commented Oct 13, 2016 •

edited

Loading

jeffcarbs Oct 13, 2016 •

edited

Loading

jeffcarbs Oct 13, 2016 •

edited

Loading

levithomason commented Oct 13, 2016 •

edited

Loading

levithomason commented Oct 14, 2016 •

edited

Loading

jeffcarbs commented Oct 15, 2016 •

edited

Loading