Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Victory to 30.0.0 #2883

Merged
merged 2 commits into from Sep 16, 2019
Merged

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented Sep 7, 2019

Victory just released a much anticipated, major update. This should help solve many of the issues folks have been noting with tooltips in #2541. For example, the new constrainToVisibleArea property will alter the position of the tooltip so that it exactly fits within the rendered SVG.

Unfortunately, this is not a trivial bump of the Victory package number. Victory introduced breaking changes that directly affect our chart components.

The x and y values passed to labels by their parent components have all been adjusted so that their values match the position of the data point they correspond to. All padding is now accounted for in the dx and dy props instead of being added directly to x and y.

This will be a breaking change for anyone who is wrapping label components and relying on the x and y props they receive, or providing their own dx / dy props. These breaking changes may take a bit of manual adjustment to correct, but we hope this change will make label positioning easier to reason about in the long run.

In order to use the constrainToVisibleArea property with pie, donut, donut utilization, and donut threshold, those components must be refactored. For example, instead of using properties such as donutHeight and donutWidth, we must use the padding property, along with height and width. It is imperative that the underlying component tooltips match the SVG container height and width -- otherwise, the tooltip's flyout position is misaligned.

These properties were introduced to help make space to render components such as legends, but this can also be done via padding. If users have not included legends with donuts; for example, chances are these changes won't affect them. Charts such as area, bar, line, & stack already use the padding approach.

The changes in this PR also include new Victory component properties, updates to the demo app, and documentation.

Fixes #2786

BREAKING CHANGES

  • All components
  • ChartPie
    • removed pieHeight & pieWidth props -- use padding prop
    • adjusted padding from 8px to 20px
  • ChartDonut
    • removed donutHeight & donutWidth props -- use padding prop
    • adjusted padding from 8px to 20px
  • ChartDonutUtilization
    • removed donutHeight & donutWidth props -- use padding prop
    • adjusted padding from 8px to 20px
  • ChartDonutThreshold
    • removed donutHeight & donutWidth props -- use padding prop
    • adjusted padding from 8px to 20px
  • ChartBullet
    • removed bulletHeight & bulletWidth props -- use bulletSize prop
  • ChartContainer
    • renamed the VictoryContainer CSS selector as pf-c-chart for specificity

The following are unused, internal only props, documented as not to be set manually. They were used internally to position donut utilization within donut threshold.

  • ChartPie
    • removed pieDx, pieDy, legendDx, legendDy
  • ChartDonut
    • removed donutDx, donutDy, legendDx, legendDy, subTitleDx, subTitleDy
  • ChartDonutUtilization
    • removed donutDx, donutDy, legendDx, legendDy, subTitleDx, subTitleDy, dx, dy
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 7, 2019

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

@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch 3 times, most recently from 59b2eb7 to 6df6857 Sep 7, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch 3 times, most recently from 20b0fe6 to d43e352 Sep 8, 2019
@dlabrecq dlabrecq marked this pull request as ready for review Sep 8, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from d43e352 to ba228da Sep 9, 2019
@tlabaj tlabaj requested review from priley86 and TheRealJon Sep 9, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch 5 times, most recently from facb9d0 to 3e7ec52 Sep 9, 2019
@christianvogt

This comment has been minimized.

Copy link

christianvogt commented Sep 10, 2019

Tooltips are being constrained and the arrow is getting cut off in some cases.

new:
image
old:
image

@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented Sep 10, 2019

Victory's constrainToVisibleArea feature is optional, but this is expected behavoir. The arrow is not being cut off, it's still pointing to the same object / point. The tooltip's center has been re-positioned to fit in the SVG, while the old example is overlapping.

The only thing we can do about that is provide more padding to accommodate the tooltip.

By default, pie / donut charts only have 8px of padding, while area, bar, etc. have 50px. Thinking we should adjust the default padding from 8 to 20 -- 50px would make the donut too small IMO.

Screen Shot 2019-09-10 at 9 14 49 AM

@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from 3e7ec52 to 888c7f5 Sep 10, 2019
@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented Sep 10, 2019

Adjusted the default padding from 8 to 20.

Variables have been updated in pf-core via patternfly/patternfly-next#2247.

Copy link
Contributor

TheRealJon left a comment

Mostly nits. There are a lot of places where the code could be more succinct. I'm not sure what this repos best practice is for things like ES6 arrow function shorthand (() => value vs () => { return value; } ), but IMO, it's useful to do this when possible. Overall, it would be nice to see more comments where there are a lot of really specific calculations being made using 'magic numbers'. Someone completely new to this repo would have a lot of trouble understanding everything that's going on. In some cases, it might even be worth defining some constants or enums for some of the default or commonly used values.

@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from 888c7f5 to 11e17f6 Sep 11, 2019
@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented Sep 11, 2019

Updated PR per feedback from @TheRealJon

@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from 11e17f6 to 0e0cfb1 Sep 11, 2019
@priley86

This comment has been minimized.

Copy link
Member

priley86 commented Sep 11, 2019

tested this downstream in Subscriptions alongside #94 just now. No issues as long as @patternfly/react-tokens is updated alongside.

cc: @cdcabrera

@priley86 priley86 requested a review from cdcabrera Sep 11, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from f5d2556 to c5be259 Sep 13, 2019
@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Sep 13, 2019

I added a commit so we get a major version bump for the package. Ignore the failing CI, it fails for the right reason!

I expect it will only do a major release for react-charts:

➜  patternfly-react git:(2786-victory-update) yarn run lerna updated
yarn run v1.16.0
$ /home/redallen/src/patternfly-react/node_modules/.bin/lerna updated
lerna notice cli v3.14.1
lerna info versioning independent
lerna info Looking for changed packages since @patternfly/react-core@3.104.0
@patternfly/react-charts
lerna success found 1 package ready to publish
Done in 1.75s.
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch 3 times, most recently from 40167a7 to 00bff8e Sep 13, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch 8 times, most recently from 3b1e170 to 1c7b7de Sep 15, 2019
dlabrecq and others added 2 commits Aug 27, 2019
CHANGES: In order to work with constrainToVisibleArea, removed the donutHeight/Width, & pieHeight/Width props — use padding prop. Replaced bulletHeight/Width with bulletSize. Default padding increased from 8 to 20px. Renamed VictoryContainer selector as pf-c-chart.

Fixes #2786
BREAKING CHANGE: upgrade to Victory 30.0.0
@dlabrecq dlabrecq force-pushed the dlabrecq:2786-victory-update branch from 1c7b7de to e42f068 Sep 16, 2019
Copy link
Member

priley86 left a comment

approved - (noting the BREAKING CHANGE lint error) is intended

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Sep 16, 2019

It should still release fine:

➜ patternfly-react git:(2786-victory-update) yarn run lerna updated
yarn run v1.16.0
$ /home/redallen/src/patternfly-react/node_modules/.bin/lerna updated
lerna notice cli v3.14.1
lerna info versioning independent
lerna info Looking for changed packages since @patternfly/react-docs@4.13.3
@patternfly/react-charts
lerna success found 1 package ready to publish
Done in 1.49s.

Merging now and will watch the release process.

@redallen redallen merged commit b16c335 into patternfly:master Sep 16, 2019
7 of 8 checks passed
7 of 8 checks passed
ci/circleci: lint Your tests failed on CircleCI
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 16, 2019

Your changes have been released in:

  • @patternfly/react-charts@5.0.0
  • demo-app-ts@3.0.0
  • @patternfly/react-integration@3.0.0

Thanks for your contribution! 🎉

@dlabrecq dlabrecq deleted the dlabrecq:2786-victory-update branch Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.