Bump victory 34.0.x

[jotak]

I'd like to bump victory charts to 34.0.x, to get a patch I did there: FormidableLabs/victory#1474

This will enable having better / more customized tooltips. Like here in kiali:

This version upgrade (33.x to 34.x) is supposedly a breaking change but I'm not sure if it can have any impact on patternfly. There's actually just a single breaking change: https://github.com/FormidableLabs/victory/blob/master/CHANGELOG.md#3400-2019-12-20
It's about using react's context API. I've built and did some smoke tests locally without seeing anything broken.

[patternfly-build]

PatternFly-React preview: https://patternfly-react-pr-3556.surge.sh

[aljesusg]

LGFM

[dlabrecq]

I've build Cost Management with 34.0, which seems to work fine.

My concern is that this will force app's to update their own dependency? For example, I'd likely need to update Cost Management's Victory packages. We install Victory mainly to define some data types; DomainTuple, VictoryStyleInterface, etc.

Perhaps we can create a peer dependency with a version of "^33.0|^34.0"?

That said, the activePoints prop would not be available in the existing ChartTooltip. We would need to add the new prop there to make it available to users.

[jotak]

I've build Cost Management with 34.0, which seems to work fine.

My concern is that this will force app's to update their own dependency? For example, I'd likely need to update Cost Management's Victory packages.

Perhaps we can change the version to "^33.0 | ^34.0"?

Not sure to understand, I don't know how Cost Management dependencies are managed, isn't it a simple dependency to PF that transitively pulls victory? So victory packages are automatically updated when you update patternfly?

That said, the activePoints prop would not be available in ChartTooltip. We would need to add the new prop there to make it available to users. In that case, we would need to update to 34.0.

This is not actually the Tooltip component that gets this new props, but the internal Flyout and Label components... which can be customized. So it doesn't necessitate changes in pf's ChartTooltip to make use of.
In Kiali we define a CustomFlyout and a CustomLabel passed to ChartTooltip:

      <ChartTooltip
        flyoutComponent={<CustomFlyout />}
        labelComponent={<CustomLabel colors={colors} />}
        constrainToVisibleArea={true}
      />

So it's in CustomFlyout and CustomLabel that we can get activePoints and make the magic happen.

[dlabrecq]

It appears that VictoryTooltip has a new activePoints prop. Are you suggesting we don't want to sync this with ChartTooltip props?

https://github.com/jotak/victory/blob/6993f657670d554c433cc17d9313bde87d07eed4/packages/victory-tooltip/src/victory-tooltip.js#L27

[jotak]

@dlabrecq sorry, yes there's this new props what I wanted to say is that it's not required to have it formalized in ChartTooltip to have the whole thing working, because this props is actually injected by the VictoryVoronoiContainer and not by the user directly. This is more like an "internal prop", as several similar prop exist in Victory, that doesn't have to be exposed to the user in ChartTooltip (actually I don't see how the user would provide it, unless by writing its own container).

If that can help, here's a workflow example:

User creates VictoryVoronoiContainer with injected tooltip+label component model as shown here #3556 (comment) => VVC creates tooltip from model (cloning), injecting activePoints => Tooltip creates LabelComponent from model, injecting activeProps => User can use this activePoints from own customized Label component. (and same for flyout component).

So the ChartTooltip here is transitory and user doesn't act upon it for activePoints.

[dlabrecq]

Talking with the PatternFly infrastructure team and this will need to wait for a breaking change release. That is, considering apps may need to update their own version of Victory as well (e.g., for customization, types, etc.)

While we update to 34.0.x, we should also update @types/victory.

[dlabrecq]

We import VictoryVoronoiContainer from the victory package, so probably don't need to add victory-voronoi-container here?

[redallen]

Breaking release change time is here! Rebase on the v4 branch and open a new PR against the v4 branch and we can take this in.