feat(Modal): Enable usage without wrapper, add custom Portal #571

jeffcarbs · 2016-09-30T07:00:04Z

In an issue (#553) I had suggested we rely more heavily on react-portal. After digging into it a bit more, I realized a few things:

It's not very actively maintained. The maintainers haven't commented on a PR in weeks if not months.
Didn't seem like they had an interest in adding much more. In order to make use of this component for other components (e.g. Popup) we may need more flexibility.
There's not a ton of code to get "Portal" functionality.

Therefore, I decided to build our own Portal loosely based on react-portal. I used their test file as a starting point to ensure we didn't lose much parity. I put it in the "addons" for now but let me know if there's a better spot for it. It has some cool features:

configurable closeOnDocumentClick and closeOnEscape props
trigger props that lets you render a node (e.g. a button) where you define the Portal so you don't need a wrapping div.
configurable openOnTriggerClick and openOnTriggerMouseOver props. There's no closeOnTriggerMouseLeave but I figured something like that would be useful for Popup/tooltips.
It's more declarative than react-portal. The open/close methods just set state and then the portal renders based on that.

I updated Modal to work with the new Portal component. There are still a few things that I need to solidify and/or figure out:

Modal now takes a portal prop, which are props to be passed to the Portal. It's not a shorthand because the Portal is not rendered like a regular element. The other option here would be to pass the props directly to the Modal and use something like _.pick(rest, _.keys(Portal.propTypes)) to figure out what to pass to Portal. Here's why I prefer the one prop:
- We can theoretically allow passing null to not render the portal (e.g. if the use wants a modal but wants to handle hiding/showing it some other way).
- It goes farther in decoupling the modal from the portal.
- The negative is that it's slightly more verbose: <Modal portal={{ trigger: <Button>Long Modal</Button> }}> vs <Modal trigger={<Button>Long Modal</Button>}>
Portal is an autocontrolled component which changes the existing usage of Modal slightly.
- Uncontrolled - you can specify a trigger and have it open on click/hover and then configure the modal to close via clickaway or escape. You can pass onOpen and onClose to the modal which will be called when the modal is opened or closed. Issue:
  - Ideally you'd be able to close the modal by interacting with some content (e.g. a button) within the modal in the uncontrolled case. Can that be possible somehow?
- Controlled - you can specify open/close state but since onOpen and onClose are only called when the modal is actually opened/closed, there are no hooks to actually control them from the outside.
  - I'm thinking that if the modal would have closed in the uncontrolled state (e.g. press escape and have closeOnEscape set) we fire a onCloseAttempt (better name?) callback. If it would've opened, we fire the onOpenAttempt.

codecov-io · 2016-09-30T07:17:59Z

Current coverage is 99.62% (diff: 100%)

Merging #571 into master will increase coverage by 0.01%

@@             master       #571   diff @@
==========================================
  Files           117        118     +1   
  Lines          1807       1862    +55   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1800       1855    +55   
  Misses            7          7          
  Partials          0          0

Powered by Codecov. Last update 21fc348...685c0b2

jeffcarbs · 2016-09-30T15:19:02Z

@levithomason @layershifter - would appreciate some thoughts on the above, specifically the open issues 😄

typehorror · 2016-09-30T21:49:23Z

Hey @jcarbo , I'm facing an issue when using the openOnTriggerFocus where the click that provides the focus might also be capture by the click listener in charge of closing the portal. We might want to make sure closing clicks on the trigger are ignored.

levithomason · 2016-09-30T21:53:28Z

Once again, thanks much. I wanted to do exactly this ever since adding the dep 👍

Modal portal prop

We could also consider providing the Modal separately from the Portal and requiring the usage to include both the Portal and the Modal. This way, the Modal is a static component. Same with the Popup. This inspired by react-bootstrap's react-overlays. It makes it possible to use any portal configuration with any component.

I think I prefer more succinct APIs, where the Modal "just works". So, we could bundle it but provide StaticModal or something for the static one. Thinking out loud...

Open Issues

Ideally you'd be able to close the modal by interacting with some content (e.g. a button) within the modal in the uncontrolled case. Can that be possible somehow?

I think we do this for specialized modals such as the Confirm. Otherwise, it is minimal wiring for the user if they want more control. I say this as it deals with modifying children (adding click handlers). The only sane way I think we should do that is via props. This then means if the user passes us some button, we can insert that into children, but they cannot then use children. This is almost exactly what the Confirm modal does. I think any use case beyond that and they can build their own Modal implementation, for now.

I'm thinking that if the modal would have closed in the uncontrolled state (e.g. press escape and have closeOnEscape set) we fire a onCloseAttempt (better name?) callback. If it would've opened, we fire the onOpenAttempt.

I've seen request/attempt prop names around, but I think they are not inline with React. Consider the <input onChange={} />, it is fired regardless of whether or not the value will actually update. So, I think change handlers should always fire when the user tries to change the state. The value to the change handler should always be the new proposed value IMO. Whether or not that value was applied to the value prop or not (controlled/uncontrolled) makes no difference.

We had to deal with this exact issue in the Dropdown via this.onChange(). We call this every time there is a user prompted change attempt such as a tab, enter, click, etc. We manually call this as there is no guarantee trySetState will be successful.

levithomason · 2016-09-30T21:54:15Z

docs/app/Examples/modules/Modal/Types/ModalBasicExample.js

-  }
-}
+const ModalBasicExample = () => (
+  <Modal portal={{ trigger: <Button>Basic Modal</Button> }} basic size='small'>


Here's why I prefer the one prop:

We can theoretically allow passing null to not render the portal (e.g. if the use wants a modal but wants to handle hiding/showing it some other way).

It goes farther in decoupling the modal from the portal.

The negative is that it's slightly more verbose: <Modal portal={{ trigger: <Button>Long Modal</Button> }}> vs <Modal trigger={<Button>Long Modal</Button>}>

You raise some good points, here are ways I think we can solve them with keeping a flat API:

We could still allow portal={false} to render without the portal. Just have a separate render method for that case.

We could provide separate versions of stateful components (ie Modal.Static).

The plus is that we get a flat API.

levithomason · 2016-09-30T21:54:43Z

docs/app/Examples/modules/Modal/Types/ModalModalExample.js

+  <Modal portal={{ trigger: <Button>Show Modal</Button> }}>
+    <Modal.Header>Select a Photo</Modal.Header>
+    <Modal.Content image>
+      <Image wrapped className='medium' src='http://semantic-ui.com/images/avatar2/large/rachel.png' />


While we're at it, would you mind making this size='medium'.

levithomason · 2016-09-30T21:55:27Z

docs/app/Examples/modules/Modal/Types/ModalScrollingExample.js

+  <Modal portal={{ trigger: <Button>Long Modal</Button> }}>
+    <Modal.Header>Profile Picture</Modal.Header>
+    <Modal.Content image>
+      <Image wrapped className='medium' src='http://semantic-ui.com/images/wireframe/image.png' />


Image size update here as well if you don't mind.

levithomason · 2016-09-30T21:59:43Z

package.json

@@ -49,7 +49,7 @@
    "classnames": "^2.1.5",
    "debug": "^2.2.0",
    "lodash": "^4.6.1",
-    "react-portal": "^2.2.1",
+    "react-dom": "^15.3.1",


Whoops, this should be in dev deps as it is used only for the doc site and tests. Otherwise, Stardust will ship with it baked into the lib.

ReactDOM is required by the Portal component:

this.portal = ReactDOM.unstable_renderSubtreeIntoContainer( this, child, this.node )

Is there a way we can get around that?

Hm, thinking it through I don't believe we can get away without it. Though, we can at least remove it from deps and just import it. It is listed in peerDependencies with a range of supported versions. This way, users can bring their own react / react-dom and we just import it, rather than bundling it (deps).

I'm not versed on the unstable API, but I wonder why the original portal didn't use ReactDOM.render() and just treat it as it's own app. I should probably watch his talk referenced in the README, https://www.youtube.com/watch?v=z5e7kWSHWTg. :/

Ah, didn't realize it was already in peer-dependencies! I'll move this back.

levithomason · 2016-09-30T22:01:29Z

src/addons/Portal/Portal.js

+    }
+  }
+
+  componentDidUpdate(prevProps, prevState) { // eslint-disable-line complexity


Very few branches in this method, is // eslint-disable-line complexity still needed?

levithomason · 2016-09-30T22:15:46Z

src/addons/Portal/Portal.js

+
+  close = () => {
+    debug('close()')
+    this.trySetState({ open: false })


Sam, propose we call onClose just before trySetState here, so the user has opportunity to update any controlled open state.

levithomason · 2016-09-30T22:18:48Z

src/addons/Portal/Portal.js

+    if (this.node) return
+
+    this.node = document.createElement('div')
+    document.body.appendChild(this.node)


This was one of the issues I had with the portal, no way to configure where it renders to. How about making document.body configurable and defaulting it to document.body?. Perhaps:

<Portal mountNode={/* a DOM node */} />

I second this request, the popup might need to appear on a different component than body (for Fluid popup). Something to consider is how to handle the case where the container where to get dynamically destroyed.

levithomason · 2016-09-30T22:25:04Z

src/addons/Portal/Portal.js

+  }
+
+  handleTriggerBlur = (e) => {
+    if (!this.props.closeOnTriggerBlur) return


Since these are all overwriting the trigger props when cloned in the render, we need to also call the trigger handler if it exists, before we exit. Probably add some tests that ensure this is the case.

Lodash's _.invoke handles this gracefully:

handleTriggerBlur = (e) => { const { trigger, closeOnTriggerBlur } = this.props _.invoke(trigger, 'onBlur', e) if (!closeOnTriggerBlur) return // ...

levithomason · 2016-09-30T22:36:30Z

src/addons/Portal/Portal.js

+    this.portal = null
+
+    const { onClose } = this.props
+    if (onClose) onClose()


I don't think we should only call event handlers if they are triggered by direct user interactions, but not by programmatic changes. This is consistent with vanilla HTML and vanilla React handling of change events:

http://codepen.io/levithomason/full/YGAXEb/

Is there anyway we could consistently pass the event (if any) responsible for closing and opening to onClose and onOpen callback ?

Yes sir, this is another reason to only use user events to trigger handlers. Programmatic changes do not have reliable events to pass back.

We should call the corresponding trigger event handler in handleTriggerClick handleTriggerFocus handleTriggerMouseLeave and handleTriggerMouseOver and pass the event. Noted above in this comment.

We should pass the event from each of those to the open/close method as well so that it can be passed to the onOpen and onClose callbacks. This way, all callbacks get all the events possible.

levithomason · 2016-09-30T22:37:43Z

src/modules/Modal/Modal.js

  }

-  handleShow = () => {
-    debug('handleShow()')
+  handleOpen = () => {


👍 thanks for the rename, open/close makes much more sense.

jeffcarbs · 2016-10-01T02:35:31Z

Ok made some updates:

onOpen/onClose now fire when the portal is initially opened/closed AND whenever anything responsible for
fixed an issue with openOnTriggerFocus where clicking the trigger would immediately close it (cc @Debrice)
invoke the trigger's event handlers (e.g. onClick) within the callbacks we're hijacking.

Still a few left to go:

Pass the triggering event as an argument to onOpen/onClose
Custom mountNode prop.
Clean up portal prop handling (see below)
Deal with Modal handleOpen/handleClose (see below)

Questions:

For mountNode, should that be a DOM node? If so, is there a propType for that? Otherwise, should it be a selector? I'm kinda leaning DOM node to keep things more sane.
Where did we land with portal props? Just pass them directly to the Modal and have a method to pick off the relevant ones (kinda like we do for inputProps inside of the Input component)?
Related to the portal props, I think coupling the modal + portal is the way to go for now. We can decide on some sort of Modal.Staticconcept on a separate PR if that's cool. Esp given the Popup is relying on this, I don't want this to get held up too much.
We're now calling onOpen and onClose always, even when nothing happened (in the controlled scenario). There's still code within Modal that needs to be called to add/remove the dimmer-related classed to the body (see https://github.com/TechnologyAdvice/stardust/pull/571/files#diff-e762b5cc3074c8997386b6de83cac7fbR102). That's currently relying on those callbacks as well, so I'm not sure how to have those only run when the modal is shown/removed. One thought is onMount/onUnmount callbacks that are called within mountPortal and unmountPortal since those are only called when the portal is actually opened/closed.

Thoughts?

jeffcarbs · 2016-10-01T04:11:09Z

Ok, wound up going with the onMount and onUnmount which wound up making more sense I think:

onOpen is called on every event that would trigger an open based on your props (e.g. openOnTriggerFocus)
onClose is called on every event that would trigger a close based on your props (e.g. closeOnEscape)
onMount is called when the portal mounts onto the DOM
onUnmount is called when the portal unmounts from the DOM

The Modal needs to listen for the mount/unmount to handle adding/removing dimmer classes.

The one thing left to do is figure out how to handle portal props. I think I'm going to just remove the portal prop and just pass-through any portal-related props from the modal to the portal.

jeffcarbs · 2016-10-01T04:34:48Z

I fixed up all the specs and examples and the API feels pretty good. Passing the props directly to Modal and having the props passed-through to Portal is definitely the way to go. So much cleaner than passing an object as a portal prop.

@levithomason - I think this is ready for final review 🙌

levithomason · 2016-10-02T01:24:59Z

Made a few trivial cleanup commits and rebased/fix conflicts with latest master. Will merge on pass. I'll be out, so will release later tonight!

levithomason · 2016-10-02T01:37:42Z

... Or I'll just do it now. Released in stardust@0.50.0

jeffcarbs mentioned this pull request Sep 30, 2016

feat(Popup): add component #570

Merged

19 tasks

jeffcarbs force-pushed the feature/modal branch from 46673b0 to 4a495db Compare September 30, 2016 07:12

levithomason reviewed Sep 30, 2016

View reviewed changes

levithomason added the ready for review label Oct 2, 2016

jeffcarbs and others added 15 commits October 1, 2016 18:23

Add Portal component

3093067

Update modal to use Portal addon

4ee10fe

Update modal examples

2df9ca6

Add specs for openOnTriggerMouseOver

86b701d

Enable close on trigger mouseleave

f3114cc

Enable open on focus, close on blur

f291ea0

Always call onOpen/onClose, always invoke overridden trigger callbacks

6debd19

Fix click event issue with openOnFocus

c553c7a

Add mount/unmount callbacks to portal

c257ba6

Enable mounting portal at configurable node

8fd3549

Pass events to onOpen/onClose, clean up

3f8110b

Remove portal prop, pass portal-related props through modal

8ea4997

fix(Modal): import Portal directly

7c385e8

fix(Portal): remove cruft imports

c75a5a1

docs(Modal): add @see Portal

685c0b2

levithomason force-pushed the feature/modal branch from 2ce70e8 to 685c0b2 Compare October 2, 2016 01:24

levithomason approved these changes Oct 2, 2016

View reviewed changes

levithomason merged commit 8e579b1 into master Oct 2, 2016

levithomason deleted the feature/modal branch October 2, 2016 01:31

jeffcarbs mentioned this pull request Oct 2, 2016

RFC: Could/should the modal rely more heavily on React.Portal? #553

Closed

jeffcarbs removed the ready for review label Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Modal): Enable usage without wrapper, add custom Portal #571

feat(Modal): Enable usage without wrapper, add custom Portal #571

jeffcarbs commented Sep 30, 2016 •

edited

Loading

codecov-io commented Sep 30, 2016 •

edited

Loading

jeffcarbs commented Sep 30, 2016

typehorror commented Sep 30, 2016

levithomason commented Sep 30, 2016

levithomason Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016

levithomason Sep 30, 2016

jeffcarbs Oct 1, 2016

levithomason Oct 1, 2016

jeffcarbs Oct 1, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

levithomason Sep 30, 2016

typehorror Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016

typehorror Sep 30, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

levithomason Sep 30, 2016

jeffcarbs commented Oct 1, 2016 •

edited

Loading

jeffcarbs commented Oct 1, 2016

jeffcarbs commented Oct 1, 2016

levithomason commented Oct 2, 2016

levithomason commented Oct 2, 2016

feat(Modal): Enable usage without wrapper, add custom Portal #571

feat(Modal): Enable usage without wrapper, add custom Portal #571

Conversation

jeffcarbs commented Sep 30, 2016 • edited Loading

codecov-io commented Sep 30, 2016 • edited Loading

Current coverage is 99.62% (diff: 100%)

jeffcarbs commented Sep 30, 2016

typehorror commented Sep 30, 2016

levithomason commented Sep 30, 2016

Modal portal prop

Open Issues

levithomason Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typehorror Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typehorror Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs commented Oct 1, 2016 • edited Loading

jeffcarbs commented Oct 1, 2016

jeffcarbs commented Oct 1, 2016

levithomason commented Oct 2, 2016

levithomason commented Oct 2, 2016

jeffcarbs commented Sep 30, 2016 •

edited

Loading

codecov-io commented Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

typehorror Sep 30, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

typehorror Sep 30, 2016 •

edited

Loading

levithomason Oct 1, 2016 •

edited

Loading

jeffcarbs commented Oct 1, 2016 •

edited

Loading