Proposal: Alternative Overlay API

[jquense]

The current overlay API has always felt a bit wonky to me, I think in part because the React ecosystem seems to have now "standardized" around specific patterns with overlays, and the RTBS overlays are showing their age a bit. Specifically, they make the declarative opening and closing of overlays more difficult than it should be, because the logic is specified indirectly via a OverlayTrigger child. It would be nice if the below could be possible without much rigmarole.

render(){
 return (<div>
  <Button onClick={e => this.setState({ show: true })}>
   Show Modal
  </Button>

  <Modal show={this.state.show} onHide={e => this.setState({ show: false })}>
     ...
  </Modal>
 </div>)
}

This doesn't require much more code than the current implementation, but is a good bit more declarative and flexible, and inline with the React ecosystem. It also doesn't require needing to create a whole new Modal class every time you want to use a Modal.

In the case of tooltips and popovers the problem is a bit harder, because the trigger is needed to position the overlay. I don't have a great solution, and maybe the current API is best for this sort of thing, but this is a possible alternative.

render(){
 return (
  <div>
    <Button onClick={e => this.setState({ show: true, target: e.target })}>
      Show Modal
    </Button>

    <Popover 
      show={this.state.show} 
      onHide={e => this.setState({ show: false })
      target={()=> this.state.target }}>
       ...
     </Popover >
 </div>)
}

These api's don't even need to be mutually exclusive, OverlayTrigger could encapsulate the above logic fairly easily, while still allowing fine grained control over the overlays if a consumer wants it.

I'll admit that a fair bit of this is a matter of opinion, but to me this strike me as more "React like" in its explicitness. Would love feedback, and any additional thoughts

[dozoisch]

👍 Would be really awesome!

[mtscout6]

Huge 👍 I've been wanting to propose such a change as well. There's a lot less mental overhead to understand this.

[taion]

This is cool. Two notes -

1. This pattern would let us avoid the context-forwarding bits of code in overlay triggers, which will be great for a lot of Flux applications (and react-router) that use context.
2. I tried to use this as the base implementation for OverlayTrigger, but it's very problematic, because OverlayTrigger doesn't actually introduce any new DOM elements, and you can't make OverlayTrigger wrap its child in a div, because its child might be some sort of inline element. See OverlayTrigger and parent-based contexts #465 (comment) - I think you need an explicit "holder" of some sort for the modal/popover.

[mathieumg]

👍

[taion]

That said, is there any way we could reduce the boilerplate with this implementation?

One nice thing with the current ModalTrigger and OverlayTrigger is that I never have to explicitly manage state with those elements - just drop them in, and they "work".

I'm not sure happy about having to explicitly deal with component state whenever I want a modal or overlay - I would prefer a pattern that could take care of that for me, as much as possible. The container is not entirely ideal, but it does buy you that convenience.

[jquense]

That said, is there any way we could reduce the boilerplate with this implementation?

I think so, I actually think the current OverlayTrigger (in some form) is a nice compromise for tooltips and popovers where you might have a bunch of them and explicitly managing that all state is annoying.

I think tho if the trigger simply wrapped up the state/handler logic instead of also managing the overlay, you get a better seperation, and the tooltips and popovers would make better primitives by themselves. One potential issue with the current Trigger api is that it takes a ReactElement instead of of a Type, so the Trigger would have to clone it (not a huge issue) to add show/hide logic

[taion]

The overlay/tooltip can't position itself without a reference to the trigger of some sort, though - per your example, it is a little awkward to get that working. My ideal tooltip API would actually look something like

<TooltipTrigger tooltip="here is a tooltip">
  <a href="...">whatever</a>
</TooltipTrigger>

Which is not dissimilar to what we have right now, just far less verbose.

[jquense]

<TooltipTrigger tooltip="here is a tooltip">
  <a href="...">whatever</a>
</TooltipTrigger>```

If we are going to have a Trigger component, I like that. That feels less like we are pushing our desire to have a common underlying implementation on users like OverlayTrigger does...even though it is essentially the same thing. I think modals should be separated out as a different sort of thing. Generally I don't think most users have so many that managing that directly in state is a problem (unlike tooltips)

not idiomatic react...but we could also do something weird like having a convention on .children order so children[0] is the trigger, [1] the tooltip content. I don't think its a great idea but throwing it out there as a possibility

<Tooltip>
  <button>open tooltip<button/>
  "here is the tooltip"
</Tooltip>

[joemcbride]

It would definitely be nice to see the more declarative style easier to do. All of our modals are externally controlled and right now we have to maintain or own ModalTrigger to accomplish this.

We are also using fluxible which uses the context, so passing that context to the components within the modal is important.

[taion]

I'm biased by the existing syntax, but I think unidiomatically using props.children like that would be a bad thing. I think an easier version of what we have right now would be good.

I like the ergonomics of my strawman Tooltip proposal. I don't like the ergonomics of the current OverlayTrigger + Overlay setup, but I wonder if we can bring the latter closer to the former. I'm very okay with modals being different, though.

Definitely it'd be an important benefit of your proposal that it'd be possible to inject the "shown" status as an external prop from a Flux framework, though I wonder if that wouldn't be possible with the triggers as well. This is more relevant for modals than for overlays by a long shot.

ETA: Corrected many poor/incorrect wordings. Sorry.

[jquense]

just spitballing some more for tooltip api options...we could go the higher order component route as well

render(){
  let TooltipTrigger = createTooltip(<span><strong>I am</strong> a tooltip</span>)

  render (
     <TooltipTrigger postion='left' trigger='focus'>
         <button/>show</button>
     </TooltipTrigger>
  )
}

Over all I think any of these options can fit into my initial proposal where Popover, Tooltip, and Modal handle rendering themselves into a layer/portal/whatever-the-word-is, and a Trigger becomes solely responsible for wiring some element to an overlay. gives you a lower level abstraction for those that want them, and a higher level one for when the convenience is worth it. The only major potential concern (I can see) with that is needing a wrapping element, but I think that can be avoided....I may have some time in the next few days, I will try and put together an initial PR and see what happens.

[taion]

Your strawman there is potentially a lot worse from a performance PoV - you're generating a new component class every time #render() gets called, which is not ideal. As a user, I'm not sure I see the benefit to doing that over

<TooltipTrigger
  position='top'
  tooltip={<span><strong>I am</strong> a tooltip</span>}
>
  <button>show</button>
</TooltipTrigger>

[jquense]

@taion question about context, is ContextWrapper currently just there to make sure the parent based context is passed to the overlay? It is just silencing the warning right now?

I have been watching the react PR's on this, there should be a proper API to handle that case in react 0.14, so that might solve some issues there as well.

[taion]

It's about silencing the warning. I'm not aware of better options under 0.13. What will 0.14 add for managing this?

[jquense]

I have complained about this concern a bunch, so I know that the react team is aware of it and giving it due priority. AFAIK there is no better way in 0.13 :/... the current iteration of the new API is this: https://github.com/facebook/react/blob/master/src/addons/renderSubtreeIntoContainer.js which is just a render() that passes along the current context

[taion]

Makes sense. Guess we'll have to deprecate the context forwarding stuff then.

[jquense]

Had some (pleasently) unexpected free time recently, and made some attempts at this; in https://github.com/react-bootstrap/react-bootstrap/tree/wip-overlays Feedback appreciated. there are a bunch of experiments in there around the triggers, and the current version is doing some odd things to avoid extra wrapping components.

obviously still not ready for prime time, no tests, no reasonable depreciations, etc.

[taion]

I haven't had time to fully look through this in depth, but it looks like a step in the right direction.

That said, I'd be extremely careful with the overlay HOC. As mentioned above, it seems like it'd be very difficult to use correctly, so we should probably note that it should not be directly used except as a module-level thing.

Also, when I get a chance this weekend, I'm going to try out something slightly different for resolving the RootClose issue.

[jquense]

yes! the goal with any of the HOC helpers is to be used only at the module level, just like you would a mixin. as you say, creating classes in a render is a big perf concern

[taion]

Reading through the code on the branch, I think there are a lot of good ideas, but I think I'd organize them somewhat differently.

1. I'd leave the Popover/Tooltip components largely as they are, without any of the render-into-overlay or positioning logic.
2. I'd add an explicit new OverlayHolder component that handles both the logic for rendering into a new layer (as needed), and for calculating the position of the contained overlay. However, since the arrow would be a property of the tooltip or popover, this would have to pass the position details for the arrow down to the component in (1). This could either be rendered in the React node tree, or into a new layer.
3. The triggers become stubs that wrap the passed-in overlays in OverlayHolder components, and manage the minimum state required to show/hide the overlay.

I think the benefits of this approach are:

1. The exposed Popovers and Tooltips are "just" plain popovers and tooltips, and are no different from any other component w/r/t positioning or anything else, and can be subbed out with minimal work. In other words, there should be almost no changes to the existing API/demos, including the bits where we're showing a plain tooltip or overlay.
2. The OverlayHolder component can contain all the messy logic that shouldn't really belong in the trigger right now, and can be used to wrap custom overlays/popovers for those managing their own state.
3. For use cases as described above with e.g. a custom popover or tooltip component, the user could just wrap said component in an OverlayHolder, without having to apply HOC functions to their custom component class just to get things to work.

[jquense]

me and @taion had a good chat about the above points in gitter the other day, and I will try most of them out and see how that feels api-wise. I did think of another concern that could have an impact on the discussion.

I think tooltips, popovers, and modals should be responsible for their own show/hide logic, instead of that being higher up in the componenl that renders them into a new subtree (OverlayHolder). The main reason for this is that when the unmount logic is higher up the tree, nothing farther down can interrupt or delay it, which puts us in a place where FadeMixin needs to clone the element and append it into the DOM to mimic the component unmounting via transition. If the hide/show logic was in lower down (happens in the subtree) we could do things like this:

 <Fade>
  { props.show && <ToolTip/> }
</Fade>

Where Fade fades a component in on mount, and fades it out completely before unmounting, so the actually component says mounted during the transition and not just a clone, but you can still write normal looking React code. The reason this doesn;t work now is taht the Overlay will just remove everything in its subtree when hidden, so the Fade component in the tooltip has no time to transition out, since it is unmounted instantly.

The downside to this approach though is that it means that something like OverlayHolder, wouldn't be able to add show/hide logic to arbitrary markup, and each would need to handle it themselves, which isn't to difficult, but is an extra step, it also creates an implicit requirement that components impliment this contract to work with the Trigger component. Alternatively we move the Fade logic out of the tooltips/popovers/modals completely, and put it in the OverlayHolder, but that feels a bit odd to me...

[taion]

I think it'd make sense to put the show/hide/fade logic in something like OverlayHolder, no? Then if I had a custom popover that I wanted to control, then I could just drop it into an OverlayHolder, which would take care of all the shared stuff, and only worry about presentation for my displayed popover itself.

Something like OverlayHolder is thematically very similar to using an HOC function - the difference between e.g.

<OverlayHolder>
  <MyPopover />
</OverlayHolder>

and

MyPopover = createOverlay(MyPopover);

is not that big - the main difference is just that the former gives you a bit more flexibility of doing the HOC wrapping wherever you want (e.g. in the render() method of the parent component), but can be awkward for passing in many arguments for HOC creation. Compare:

http://martyjs.org/guides/containers/index.html
http://alt.js.org/docs/components/altContainer/

[jquense]

I think that the Fade logic belongs wherever the hide/show logic is, so OverlayHolder is a reasonable place for it, yeah. The one thing that is a bit weird about that is that transitions are done by adding .fade and .in classes to the provided overlay, so at least as far as css is concerned the overlay Component needs to make sure those classes do something. Tho I guess that isn't any different then the way the current FadeMixin works...

[jquense]

Most recent updates adds a Portal (OveralyHolder) Component, named as such in an attempt to try and get a bit closer to the language the React ecosystem is using for this pattern...I like it you can see the updated examples for usage.

I left the Positioning logic out of it for now. It still feels awkward to me for that to be with Layer/Overlay/Portal logic, since it creates an implicit contract between it and its child, passing positioning data as props to it, which to my mind limits its potential uses. Modals for example don't need positioning coordinates, and especially don't need arrow offset coords :P

If the current HoC for positioning feels out of line with the rest of the library we could always just make a <Positioned> Component instead, that way custom popovers things would be easy enough.

[taion]

I think the Portal has to inject the arrow positioning props to the child, because only the child knows how the arrow is rendered.

I think the Portal should handle the rest of the positioning itself (by styling the div it embeds its child into or whatever) and not pass that kind of stuff down to the child. So you'd need a slightly different OverlayPortal v ModalPortal or whatever, since only the former worries about target and positioning.

[jquense]

closed with #855