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 BarGroup #870

Merged
merged 2 commits into from
Oct 15, 2020
Merged

new(xychart): add BarGroup #870

merged 2 commits into from
Oct 15, 2020

Conversation

williaster
Copy link
Collaborator

@williaster williaster commented Oct 14, 2020

TODO

🚀 Enhancements

This adds a BarGroup series to @visx/xychart which supports tooltips, negative values, and horizontal rendering like other *Series. It updates the /xychart demo to include it.

@kristw @techniq

@@ -56,28 +58,48 @@ export default function Example({ height }: Props) {
numTicks={numTicks}
/>
{renderBarStack && (
<g fillOpacity={renderLineSeries ? 0.5 : 1}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

g element wasn't used

const groupScale = useMemo(
() =>
scaleBand<string>({
domain: [...dataKeys], // @TODO order
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

reminder to myself to add this prop


// try to figure out the 0 baseline for correct rendering of negative values
// we aren't sure if these are numeric scales or not ahead of time
const maybeXZero = xScale(0) ?? 0;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

some of this is duplicated in BarSeries. I'm going to focus on animated bars next, and will try to remove some of the duplication.

@coveralls
Copy link

coveralls commented Oct 14, 2020

Pull Request Test Coverage Report for Build 214

  • 53 of 56 (94.64%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.3%) to 59.468%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/components/series/BarGroup.tsx 49 52 94.23%
Totals Coverage Status
Change from base Build 208: 1.3%
Covered Lines: 1448
Relevant Lines: 2335

💛 - Coveralls

<g className="visx-bar-group">
{registryEntries.map(({ xAccessor, yAccessor, data, key }) =>
data.map((datum, index) => {
const barLength = horizontal
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to suggestion on barstack, these conditions can be moved outside to save n times redundant check.

@williaster williaster force-pushed the chris--xychart-stackedbar-tests branch from 406603f to 6ec13a9 Compare October 15, 2020 23:02
Base automatically changed from chris--xychart-stackedbar-tests to chris--xychart-stackedbar October 15, 2020 23:03
Base automatically changed from chris--xychart-stackedbar to master October 15, 2020 23:20
new(demo/xychart): add BarGroup

new(xychart/BarGroup): add mouseover/out

fix(demo/xychart): better ux

new(xychart/BarGroup): add sortBars

internal(xychart/BarGroup): add perf improvements
* test(xychart/mocks): add @visx/event mock

* test(xychart): add BarGroup tests

* test(xychart): add mousemove/out tests to all series

* test(xychart): remove .test. from setupTooltipTest filename

* lint(xychart/setupTooltipTest)
@williaster williaster merged commit 3d89e08 into master Oct 15, 2020
XYChart automation moved this from Reviewable to Done Oct 15, 2020
@williaster williaster deleted the chris--xychart-grouped-bar branch October 15, 2020 23:40
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
@r0st1kuzh
Copy link

@williaster hello. can I use this in the latest version of library?

@williaster
Copy link
Collaborator Author

Hey @r0st1kuzh , sorry we have not published the @visx/xychart package yet as it's still under active development and I don't want to run the risk of breaking changes to the API just yet.

It should be ready for publishing in the next couple of weeks, tho. I created #884 to track it (or you can follow the xychart project for even more details)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
XYChart
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants