Rewrite V4 for React 16 #157

tajo · 2017-07-30T06:58:51Z

EDIT 10/1/2017:

This is an umbrella ticket for new v4. This library got a complete and long deserved overhaul. The biggest changes:

split of the current Portal component into two components (stateless and stateful)
support of React v16 Portal API
support drop for React v15, it would be too much work and the switch from v15 to v16 should be easy for most of projects anyway

Check the main README.md. It documents the new components and their APIs. I am really interested in your feedback! I feel that function as a child is the right pattern for the stateful <PortalWithState/> component and provides nice level of flexibility.

You can install it as:

yarn add react-portal@next

Roadmap for stable V4:

Test and stabilize the API, possibly release more betas. I need your feedback!
Add back unit tests. They need a rewrite too.
Release notes, explaining how to migrate from v3 to v4.

I would like to release the stable v4 version in a month (November 1st).

The text was updated successfully, but these errors were encountered:

petejodo · 2017-09-07T19:12:36Z

In case it isn't common knowledge, react 16 is including a new API for portals. May be useful to take that into consideration

eliseumds · 2017-09-12T03:17:16Z

@petejodo and it's now official! React.createPortal

gaearon · 2017-09-12T10:56:46Z

It lives on ReactDOM, not React. :-)

petejodo · 2017-09-12T17:56:12Z

I've been thinking a lot about modals recently. If we have some Modal component that uses react-portal, there's some dichotomy between how that modal works and its actual design purpose. Lets say you want to do login via a modal on your site.

Now let's also say you have multiple touch points on the page that would trigger the login modal. You create some LoginModal component that handles rendering the modal so that anyone can easily include it. Now, the natural way of consuming it is to put it next to what I call the "actuator" which is usually just some <button />.

On a site where you have multiple developers, you may end up having code that looks like this then:

// Feature1.jsx
const Feature1 = props => (
  <div>
    <SomeContent />
    <button onClick={props.openAuthModal}>Login to edit content</button>
    <LoginModal isOpen={props.authOpen} />
  </div>
);

// Feature2.jsx
const Feature2 = props => (
  <div>
    <SomeMessageThread />
    <button onClick={props.openAuthModal}>Login to add message</button>
    <LoginModal isOpen={props.authOpen} />
  </div>
);

// Parent.jsx
class Parent extends React.Component {
  constructor(props) {
    this.openAuthModal = this.openAuthModal.bind(this);
    this.state = { authOpen: false };
  }

  openAuthModal() {
    this.setState(() => ({ authOpen: true }));
  }

  render() {
    return (
      <div>
        <Feature1 authOpen={this.state.authOpen} openAuthModal={this.openAuthModal} />
        <Feature2 authOpen={this.state.authOpen} openAuthModal={this.openAuthModal} />
      </div>
    );
);

The obvious issue is, when Parent's state is { authOpen: true}, then 2 divs get appended to <body /> and 2 modals gets rendered, 1 for each appended <div>. That's really because the modal should be co-located with the state that determines whether it's open or not, which gives you 2 options.

Move the modal component up the tree into Parent from the Features in order to co-locate it with the state that manages it and just pass the handler that opens the modal to the Features.
Move the state down the tree, so each Feature has to implement it's own state to manage their own instance of the modal.

Each option has its own issues though.

For option 1, what happens over time is that all modals will get pulled to the top of the tree or if you use something like react-router, to the top of your route. This ends up being a bit ugly:

const MyRoute = () => (
  <div>
    {/* route contents */}
    <LoginModal />
    <TFAModal />
    <PostMessageModal />
    <EditContentModal />
    {/* etc... */}
  </div>
);

Having to do this defeats the purpose of react-portal I feel. Note that this also applies if you manage modals' states via redux.

For option 2, one issue is when you want to open the modal on page load, for example; which instance of the modal should be rendered? You could create a PageLoad component that has it's own instance of the modal which manages the modal for that case but that doesn't sound very appealing to me. There's other situations where this occurs too but it seems to be related to the same thing which is, what instance of the modal is the true instance

All of this becomes even more complicated when you want to create flows for modals. For example, maybe sometimes LoginModal should continue on to a TFAModal (two-factor auth). You then have to make a decision on whether you just change the contents within the modal or close the first modal and then open the 2nd.

If you change the content within the modal that means you can't animate the modal itself for the transition. If you close the 1st and then open the 2nd modal, depending on how you handled the original issue, causes its own headaches.

I think this all stems from the disconnect between the implementation of modals and what they actually are from a design concept. Modals are essentially singletons from a design standpoint. While you could have a set of components with their own data and each needs to open a modal, only one modal can be open at a time (again, from a design standpoint). So what really should be happening, is the props get portal'd to a single LoginModal, for example, rather than portal-ing the entire component. And since only one modal should be rendered at a time, there should really only be a single isOpen state value in the entire app.

While react-portal isn't meant to be specifically about modals, I believe these issues pop up into other design patterns where portals are useful. I agree with there being a simple core Portal component, but there should also be something like portal-tools that help handle the above issues.

Also, while I was thinking about this, I couldn't help but think of the set of all modals on a page as a type of finite state machine and have been playing with the idea of implementing it like such.

Any discussion on this would be very useful.

gaearon · 2017-09-12T21:34:01Z

If I understand the way we use them at Facebook correctly, we have <Modal> components that are all declarative, but their implementation delegates to a plain JS module that keeps track of which modal is currently active, focus/blur behaviour, keyboard shortcuts, borders, etc. That module sends events to Modal components telling them whether they should render (or are inactive), and to which DOM node. Happy to answer more questions.

tajo · 2017-09-12T22:41:17Z

@petejodo That's a good note. I would probably use the first solution - move the modal to common parent. If there's a modal that needs to be open from multiple places, use redux and keep it somewhere near the app root.

So what really should be happening, is the props get portal'd to a single LoginModal, for example, rather than portal-ing the entire component.

Hm, I am not entirely following this. I think you can do this already? If you don't want to portal the whole component but only the props, you can use redux (or common state)?

And since only one modal should be rendered at a time

That seems like a strong assumption (especially if modal === portal).

Are there some specific changes in react-portal that could better support your use-case? Maybe react-gateway is a better fit if you don't mind context based solution with targets & sources.

petejodo · 2017-09-13T19:34:20Z

A colleague of mine and I were discussing this one day and figured we wanted a solution similar to what @gaearon said they do at facebook. Curious as to why it's done outside of React though. I assume it's some external requirement or just some older JS module that pre-dates React.

@gaearon, if you had multiple LoginModal components in separate parts of the tree does the plain JS module consider those as different modals and therefore tracks them separately?

@tajo sorry I was kind of spitting out common issues I've come across, without any consideration to actual features that would potentially make it into react-portal. I'll add another response later with features I think would be useful when I get the time to think about it even though I suspect what I'm looking for may be outside the scope of this package.

gaearon · 2017-09-13T23:12:45Z

Curious as to why it's done outside of React though. I assume it's some external requirement or just some older JS module that pre-dates React.

Yea, we have a whole system specifically for this that predates React.

gaearon · 2017-09-13T23:13:49Z

@gaearon, if you had multiple LoginModal components in separate parts of the tree does the plain JS module consider those as different modals and therefore tracks them separately?

It tracks them separately but I think it can enforce that only one is visible. I’m not sure really. Modals can specify options and maybe there’s a way to specify that some modal should always be “alone” on the screen. I can check later.

Justkant · 2017-09-27T10:36:40Z

@petejodo That's a good note. I would probably use the first solution - move the modal to common parent. If there's a modal that needs to be open from multiple places, use redux and keep it somewhere near the app root.

@tajo, In this solution I feel like portals aren't needed but correct me if I'm wrong.
If you move the portal near the app root (which is a good idea if you need it open from multiple places), the portal become useless. At least in my mind you don't need portals anymore.

tajo · 2017-10-01T21:24:00Z

^^ The main post was updated. Please read and help with testing!

tajo · 2017-10-01T21:40:25Z

^^ Sorry for that to all contributors. V3 was a mess and impossible to maintain. I feel much much better about V4. (Thanks React team for the new API!) Let's jumpstart this project!

bradleyayers · 2017-10-01T23:43:11Z

You probably want to update the logic in https://github.com/tajo/react-portal/blob/master/src/PortalWithState.js#L69. It should use React event propagation rather than DOM propagation, so that nested portals work correctly.

petejodo · 2017-10-02T04:22:41Z

@tajo thanks for the work for v4! I said I would respond later with tools I'd want for handling modals so I figured I'd close that loop.

My goal is to be able to use modals as declaratively as possible so what I came up with is a sort of combo of this package and react-gateway.

You'd place the Modal component where you need it. This would be next to a button component for example. If the Modal component is mounted, that means it's rendered, no isOpen prop. Instead a single local react state boolean can be used to determine whether to render it or not

<Foo>
  <button>Open Modal</button>
  <Modal>
    <Bar />
  </Modal>
</Foo>

Then there'd be some ModalStage component near the top of the app.

<App>
  <ModalStage />
  <Foo />
</App>

Now the Modal component acts a lot like the Gateway component from react-gateway where it adds its children to a sort of cache which ends up as the ModalStage component's children.

Now ModalStage itself acts like react-portal which would create a portal to some DOM node and render its children to it. The ModalStage could even handle the "scrim" part and closeOnOutsideClick too potentially. It would also have to handle edge-cases such as when 2 separate Modals are rendered, which it could render something like the 1st of its children or throw an error in development. You'd also have to have a Provider component or some global cache to handle adding and removing the ModalStage's children.

This I believe would allow easily constructing multi-step modal flows by using a switch-case and local react state. You could even have a Switch component which takes the prop on which could be a boolean or a number. If it's false, no children are rendered. If it's true, all children are rendered. If it's a number i, it will render the child in the i-th position:

<Foo>
  <Switch on={this.state.currentStep}>
    <Modal>One</Modal>
    <Modal>Two</Modal>
  </Switch>
</Foo>

This all seems very out-of-scope for this package so you can disregard it for the sake of this package but I'm curious what your opinion is of that idea?

Also @gaearon, as someone who is familiar with react internals, I'm curious what your opinion is of the method that react-gateway uses to portal components, which is essentially a React-to-React portal rather than a React-to-DOM portal?

edit also sorry for hijacking this issue thread, maybe this is not the appropriate place for this since it should be about the v4 rewrite

ianstormtaylor · 2017-10-02T08:06:12Z

@tajo would you consider putting portal-with-state into its own separate package? It would save on file size, and would greatly reduce the complexity of docs/etc. for this package. And it would nicely leave this package as the super simple, canonical portal component.

Not that it's a huge deal obviously, but it just seems like Portal is the nice, simple, build-complex-logic-on-top-of-this component, that in my opinion everyone should be using. And PortalWithState is really like a shim for the old practices that are not as well designed, and that are probably too much magic.

tajo · 2017-10-02T10:41:49Z

@bradleyayers Example?

@petejodo Looks reasonable if you need to handle complex modal flows. You can still use v4 <Portal /> to build <ModalStage />.

@ianstormtaylor File size is not a concern since there's now ES export and tools like Webpack will do tree-shaking out of the box. Agree, that most people should use just <Portal> and I might to make it more obvious in the README. But for now, I don't want to pay the cost of splitting this library into multiple packages.

jalopez · 2017-10-02T11:05:40Z

I have found an issue in 4.0.0-beta.1 with react 16. I don't know if you want me to open a new issue or report in this thread.

The problem when closing <PortalWithState> either with Esc key or by clicking in window. It works, but a react warning is being printed in console:

warning.js:33 Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by React and is not a top-level container. Instead, have the parent component update its state and rerender in order to remove this component.

tajo · 2017-10-02T12:33:50Z

@jalopez If you can create a ticket that would be great! I would like to keep this thread for discussion about overall design and progress towards stable release than specific issues.

tajo · 2017-10-04T23:02:29Z

@jalopez Fixed and released as v4.0.0-beta.2

tajo · 2017-10-21T22:52:54Z

4.0.0-rc.0 released. Test coverage is still not ideal since enzyme doesn't support portals yet. However, we fixed some bugs and there was no pushback against the new API. So I am still planning to release stable version on November 1st.

imevro · 2017-10-24T11:40:47Z

And what about className?

tajo · 2017-11-14T18:16:04Z

@evgenyrodionov Can you please open a new issue for that? I still don't understand why people want className for the root div.

tajo added this to the v4 milestone Jul 30, 2017

tajo changed the title ~~Rewrite V4~~ Rewrite V4 for React 16 Sep 12, 2017

tajo closed this as completed Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite V4 for React 16 #157

Rewrite V4 for React 16 #157

tajo commented Jul 30, 2017 •

edited

Loading

petejodo commented Sep 7, 2017 •

edited

Loading

eliseumds commented Sep 12, 2017

gaearon commented Sep 12, 2017

petejodo commented Sep 12, 2017

gaearon commented Sep 12, 2017

tajo commented Sep 12, 2017

petejodo commented Sep 13, 2017 •

edited

Loading

gaearon commented Sep 13, 2017

gaearon commented Sep 13, 2017

Justkant commented Sep 27, 2017

tajo commented Oct 1, 2017

tajo commented Oct 1, 2017

bradleyayers commented Oct 1, 2017

petejodo commented Oct 2, 2017 •

edited

Loading

ianstormtaylor commented Oct 2, 2017

tajo commented Oct 2, 2017

jalopez commented Oct 2, 2017

tajo commented Oct 2, 2017

tajo commented Oct 4, 2017

tajo commented Oct 21, 2017

imevro commented Oct 24, 2017

tajo commented Nov 14, 2017

Rewrite V4 for React 16 #157

Rewrite V4 for React 16 #157

Comments

tajo commented Jul 30, 2017 • edited Loading

petejodo commented Sep 7, 2017 • edited Loading

eliseumds commented Sep 12, 2017

gaearon commented Sep 12, 2017

petejodo commented Sep 12, 2017

gaearon commented Sep 12, 2017

tajo commented Sep 12, 2017

petejodo commented Sep 13, 2017 • edited Loading

gaearon commented Sep 13, 2017

gaearon commented Sep 13, 2017

Justkant commented Sep 27, 2017

tajo commented Oct 1, 2017

tajo commented Oct 1, 2017

bradleyayers commented Oct 1, 2017

petejodo commented Oct 2, 2017 • edited Loading

ianstormtaylor commented Oct 2, 2017

tajo commented Oct 2, 2017

jalopez commented Oct 2, 2017

tajo commented Oct 2, 2017

tajo commented Oct 4, 2017

tajo commented Oct 21, 2017

imevro commented Oct 24, 2017

tajo commented Nov 14, 2017

tajo commented Jul 30, 2017 •

edited

Loading

petejodo commented Sep 7, 2017 •

edited

Loading

petejodo commented Sep 13, 2017 •

edited

Loading

petejodo commented Oct 2, 2017 •

edited

Loading