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

new(xychart): add Animated(BarSeries, BarStack, BarGroup) #873

Merged
merged 9 commits into from
Oct 22, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Oct 14, 2020

TODO

🚀 Enhancements

This PR

  • adds react-spring animated variants of BarSeries, BarStack, and BarGroup
    • this was done by factoring out a BaseBarSeries, BaseBarStack, and BaseBarGroup, and then creating the equivalent BarSeries/AnimatedBarSeries, etc. by passing Bars or (new) AnimatedBars as the BarsComponent (this approach was used for Grid/AnimatedGrid, and Axis/AnimatedAxis
  • adds react-spring as a peerDependency (this will only be required if you use any of the AnimatedGrid/Axis/BarSeries/BarStack/BarGroup components)

Updated the demo to use these by default. I tested enter/update/leave transitions by tweaking several demo config options.

@kristw @techniq

orientation === 'left' || orientation === 'right'
? margin?.[orientation]
: undefined,
...props.tickLabelProps?.(value, index),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the only thing that was changed here is that all ...axisStyles.tickLabel styles are applied by default and the user-specified styles (props.tickLabelProps) are overrides. this makes it way easier to override a single style or two.


return (
<g className="visx-bar-group">
<BarsComponent bars={bars} horizontal={horizontal} xScale={xScale} yScale={yScale} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is all the same as previous BarGroup, bars are just passed to BarsComponent now.


return (
<g className="vx-bar-series">
<BarsComponent bars={bars} horizontal={horizontal} xScale={xScale} yScale={yScale} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is all the same as previous BarSeries, bars are just passed to BarsComponent now.


return (
<g className="visx-bar-stack">
<BarsComponent bars={bars} horizontal={horizontal} xScale={xScale} yScale={yScale} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is all the same as previous BarStack, bars are just passed to BarsComponent now.

@coveralls
Copy link

coveralls commented Oct 14, 2020

Pull Request Test Coverage Report for Build 309632235

  • 150 of 167 (89.82%) changed or added relevant lines in 13 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.4%) to 59.065%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/components/series/AnimatedBarGroup.tsx 0 1 0.0%
packages/visx-xychart/src/components/series/AnimatedBarSeries.tsx 0 1 0.0%
packages/visx-xychart/src/components/series/AnimatedBarStack.tsx 0 1 0.0%
packages/visx-xychart/src/components/series/private/BaseBarGroup.tsx 49 52 94.23%
packages/visx-xychart/src/components/series/private/AnimatedBars.tsx 0 11 0.0%
Totals Coverage Status
Change from base Build 309618472: -0.4%
Covered Lines: 1454
Relevant Lines: 2355

💛 - Coveralls

@williaster williaster force-pushed the chris--xychart-grouped-bar-tests branch from 3d54674 to 8be48e7 Compare October 15, 2020 23:24
Base automatically changed from chris--xychart-grouped-bar-tests to chris--xychart-grouped-bar October 15, 2020 23:25
Base automatically changed from chris--xychart-grouped-bar to master October 15, 2020 23:40

return barStack.map((bar, index) => {
const barX = getX(bar);
if (!isValidNumber(barX)) return null;
Copy link
Member

Choose a reason for hiding this comment

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

should we log a warning here? or are there cases where devs would expect bar{X|Y} to not be a valid number?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fair point. I have an item in the project tracker to do a sweep across series to make sure they handle nulls / missing values properly so will think about it for that. my sense is that we don't need a warning since null values likely are common.

@williaster williaster added this to the 1.1.1 milestone Oct 22, 2020
@williaster williaster merged commit 7514240 into master Oct 22, 2020
@williaster williaster deleted the chris--xychart-animated-bar branch October 22, 2020 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants