Skip to content

Conversation

@TheRealJon
Copy link
Contributor

@TheRealJon TheRealJon commented Apr 30, 2019

fixes #1898

What:
Update @types/victory, victory, victory-core, and hoist-non-react-statics to latest and remove extra type definition for the title prop in ChartLegend. The latest victory types include an accurate definition for this prop.

Victory changelog for reference:
https://github.com/FormidableLabs/victory/blob/master/CHANGELOG.md

@TheRealJon TheRealJon requested review from dgutride and dlabrecq April 30, 2019 21:26
@patternfly-build
Copy link
Collaborator

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

@codecov-io
Copy link

codecov-io commented Apr 30, 2019

Codecov Report

Merging #1899 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1899   +/-   ##
=======================================
  Coverage   82.62%   82.62%           
=======================================
  Files         623      623           
  Lines        6870     6870           
  Branches       93       93           
=======================================
  Hits         5676     5676           
  Misses       1154     1154           
  Partials       40       40
Flag Coverage Δ
#patternfly3 84.87% <ø> (ø) ⬆️
#patternfly4 79.37% <ø> (ø) ⬆️
#patternflymisc 95.68% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6886dbb...08862a2. Read the comment docs.

Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing a bunch of build errors in Cost Management with these changes, specifically around VictoryStyleInterface references. I suspect the VictoryStyleInterface property types have changed?

Screen Shot 2019-04-30 at 8 56 04 PM

If we're going to take the latest @types/victory, we should probably take the latest victory and victory-core packages to ensure compatibility?

When I update all three packages, and/or remove my VictoryStyleInterface references, Cost Management builds successfully.

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we can remove title ... also looks like this a breaking change for Dan.

@dlabrecq
Copy link
Member

dlabrecq commented May 1, 2019

The title prop can be removed, but believe we need to update all 3 victory packages to ensure types are is in sync.

@dlabrecq
Copy link
Member

dlabrecq commented May 1, 2019

Discovered that building react-charts with the latest Victory packages destroyed the bullet charts we created in Cost Management. I'll try to find a solution, but this will be a breaking change for Cost Management with no apparent workaround.

Screen Shot 2019-05-01 at 11 13 38 AM

@TheRealJon
Copy link
Contributor Author

TheRealJon commented May 1, 2019

@dlabrecq

Discovered that building react-charts with the latest Victory packages destroyed the bullet charts we created in Cost Management. I'll try to find a solution, but this will be a breaking change for Cost Management with no workaround.

See the following in the changelog for v32.0.0:

Breaking Changes

Most Horizontal Charts The change in how props with x and y values are treated (i.e. scale, domain, etc) will be a breaking change for most horizontal charts. In most cases, reversing the x and y values of props you are providing to your horizontal chart will be sufficient to correct the change. For example:

<VictoryChart horizontal scale={{ x: "log" }} domain={{ y: [4, 9] }}>
  <VictoryBar
    data={[
      { x: 5, y: 0.1 },
      { x: 6, y: 1 },
      { x: 7, y: 10 },
      { x: 8, y: 100 }
    ]}
  />
</VictoryChart>

Should be changed to:

<VictoryChart horizontal scale={{ y: "log" }} domain={{ x: [4, 9] }}>
  <VictoryBar
    data={[
      { x: 5, y: 0.1 },
      { x: 6, y: 1 },
      { x: 7, y: 10 },
      { x: 8, y: 100 }
    ]}
  />
</VictoryChart>

Props affected by this change are: scale, domain, maxDomain, minDomain, domainPadding, and categories

Horizontal Charts with Event Containers Dimension props such as brushDimension have changed so that they always refer to the dimension of the target variable (x for the independent variable, and y for the dependent variable). For example, a VictoryBrushContainer component with brushDimension="x" will move and expand only in the independent dimension regardless of whether the chart is horizontal.

Props affected by this change are: brushDimension, cursorDimension, selectionDimension, voronoiDimension, and zoomDimension

Horizontal Charts with Custom dataComponents The position values (i.e. x, y, x0, y0) supplied to custom dataComponents from components like VictoryChart will be scaled for the layout of the horizontal chart. Users who rely on these values may need to flip them or adjust them depending on their use case

Horizontal VictoryBoxPlot Previously VictoryBoxPlot required data to be flipped (x values flipped with y values) in order to plot horizontal charts. This is no longer required, and providing data in this format will no longer work correctly. To correct for this change, it should be sufficient to flip the data used in horizontal charts

@TheRealJon TheRealJon changed the title fix(types): Update @types/victory package to latest fix(types): Update @types/victory, victory, victory-core, and hoist-non-react-statics packages to latest May 1, 2019
@TheRealJon TheRealJon changed the title fix(types): Update @types/victory, victory, victory-core, and hoist-non-react-statics packages to latest fix(types): Update @types/victory, victory, victory-core, and hoist-non-react-statics packages to latest May 1, 2019
Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @TheRealJon. Although, it turned out to be a little more than just swapping the x & y coordinates.

Certain style properties no longer work for bars (e.g., width). And we can no longer render VictoryBar on both the horizontal and vertical axes. Don't know if that's by design, but it forces all bars to be on the same axis. As a result, our bullet chart cannot render thresholds properly.

We can use a vertical VictoryLine (i.e., PD ChartLine) to represent the threshold instead, but lose the ability to have a tooltip for that component.

That said, we will find a workaround or just omit the thershold tooltip.

Update: Found a workaround for the tooltip -- should be ok.

@tlabaj tlabaj added PF4 labels May 2, 2019
@dlabrecq dlabrecq requested a review from dlabaj May 6, 2019 19:29
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tlabaj tlabaj merged commit d3e2ec7 into patternfly:master May 7, 2019
@TheRealJon TheRealJon deleted the update-types branch June 26, 2019 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TypeScript Error for ChartLegend Component

6 participants