combining containers

[chrisbolin]

Introduces a combineContainerMixins function that (wait for it) combines containers. This required a 2-line change to each of the containers, so that they export both the container (as before) and a mixin function that lets the container inherit from any class.

As an example, the VictoryVoronoiZoomContainer is included in this PR. It's entire code is

import { combineContainerMixins } from "./combine-container-mixins";
import { voronoiContainerMixin } from "./victory-voronoi-container";
import { zoomContainerMixin } from "./victory-zoom-container";

export default combineContainerMixins(voronoiContainerMixin, zoomContainerMixin);

Again, the *ContainerMixin are very similar to the Victory*Container classes; all of the magic happens in combineContainerMixins(A, B). This function combines the classes; the interesting bits are for getChildren and defaultEvents.

• The combined getChildren will call A's getChildren() and insert the resulting children into props calling B's getChildren().
• The combined defaultEvents is more complex. It attempts to resolve any conflicts between A and B's defaultEvents by concatenating the produce mutations.

The results are pretty fun...

Voronoi + Zoom Containers

Voronoi + Brush Containers

Voronoi + Selection Containers

Zoom + Selection Containers

(see WIP before for a note on this)

Work in progress. Still need to figure out...

• should we include VictoryVoronoiZoomContainer rather than make people build their own? Building is easy (2 lines of code) but it's nice to have an example.
• is the word mixin ok?
• we still need to have a way to inject custom handling. e.g. on the Zoom + Selection Containers, the onMouseDown event actually does conflict: you can't drag a zoomed component and make a selection at the same time (well you can, but it's no fun).

(also includes individual page titles for the demos, because I needed that :) )

[boygirl]

@chrisbolin this is so awesome! I love the simplicity of this approach.

I couple thoughts:

• We should be exporting all of the mixins from /index
• I'm fine with "mixin" even though it's a little overloaded. I can't think of better naming here.
• I'm really on the fence about exporting VictoryVoronoiZoomContainer. I think it's a common set of behaviors that people will want, but it kind of makes me cringe.
• is combineContainerMixins limited to two mixins? Should it support more, or is that just a giant unnecessary foot gun?
• Is zoom + onMouseDown the only conflict? Maybe we can add an allowPan prop the same way we have allowZoom?

Even though it would be a breaking change, the architecture is making me think that something more like <VictoryContainer behaviors={["zoom", "voronoi"]} zoomDomain={{x: [0, 100]}}.../> might be a better api. I guess we lose props validation if we do that though. Hmm.

[chrisbolin]

@boygirl I love the idea of simplifying the API. Instead of

<VictoryContainer behaviors={["zoom", "voronoi"]} zoomDomain={{x: [0, 100]}}.../>

we could do...

<VictoryContainer
  voronoi
  zoom={{
    zoomDomain: {x: [0, 100]},
  }}
/>

VictoryContainer would have optional props zoom, selection, voronoi, and brush. These props could have the type Boolean || Object. A Boolean true value would a the behavior with the default options; an object would be the shape of the behavior’s options (currently the container’s options) and add the behavior. For example, the shape of the the zoom prop would be

{
  zoomDomain,
  minimumZoom,
  onDomainChange,
  clipContainerComponent,
  allowZoom,
  dimension,
}

All the same defaults would still apply, so
<VictoryContainer zoom /> and <VictoryContainer zoom={{}} />
would be identical.

What do you think?

[chrisbolin]

Alright, I decided to take a slightly different path from what I mentioned up there. This uses a simple createContainer() method that optionally takes behaviors (“zoom”, “brush”, etc). It is not a breaking change.

For example, we can eliminate VictoryVoronoiZoomContainer, as uses can create it with just

const VictoryVoronoiZoomContainer = createContainer("zoom", "voronoi”);

The Container behaviors can still be mixed in with the custom user containers with combineContainerMixins for advanced use-cases.

I wanted to abstract this further into a component with a behaviors prop, but I hit a strange issue. If this solution doesn’t work I can keep pursuing that, though.

Currently createContainer() is limited to two behaviors. I could definitely make it work for more, but it might be asking for trouble.

I still have to add exports to index (assuming we like this approach).

[boygirl]

@chrisbolin I like the simplicity of this approach a lot. A few things to clarify. This might just come down to documentation

• Does the order in which you supply the behaviors matter? Could it potentially in cases of conflicting event handlers?
• How can we make it clearer that only two behaviors can be combined? Warn with more than params?
• Warn when unsupported strings are supplied.

[chrisbolin]

I think we can make all that more clear in the code. I'll whip something up

[chrisbolin]

Alright, I added some helpful warnings for the two cases you mentioned.

[chrisbolin]

As for order-of-behaviors... I found an annoying quirk. Surprisingly, it has nothing to do with events, though! It has to do with clipping in getChildren...

createContainer("zoom", "voronoi") is better than createContainer("voronoi", "zoom") because in rare cases, the zoom clip container can clip the voronoi tooltip. This does not happen on the Scatter plot demo, but does happen on the Line demo (I don't yet know why there is a difference).

This is caused by how I implemented getChildren in createContainer...

    getChildren(props) {
      const children = instanceA.getChildren.call(this, props);
      return instanceB.getChildren.call(this, {...props, children});
    }

A is called before B, and hence why createContainer("zoom", "voronoi") is preferable. An annoying corner case for sure. Should we still move ahead, or try and squash it? (Off the top of my head, I can't think of a way to do it that doesn't require re-designing all of the specialty container's getChildren methods and breaking them into separate steps. And even still, there might be corner cases.)

[chrisbolin]

@boygirl I pushed some changes...

• completely removed VictoryVoronoiZoomContainer
• made a demo that shows of createContainer on two different combined containers (zoom+voronoi, and brush+voronoi),
• exported createContainer, plus all the mixins and combineContainerMixins (for power users that want to combine their own containers with one of the provided four).

I'll merge in master now and go from there.