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 BarSeries #808

Merged
merged 13 commits into from
Oct 1, 2020
Merged

new(xychart): add BarSeries #808

merged 13 commits into from
Oct 1, 2020

Conversation

williaster
Copy link
Collaborator

This PR adds BarSeries to the xychart package and refactors / fixes other issues in the process

🚀 Enhancements

  • adds BarSeries component

  • adds withRegisteredData HOC which swaps data/xAccessor/yAccessor props for those in the DataRegistry in context. This makes *Series implementation more straight forward because they should always use values from context so they are in sync with the context scales.

  • re-writes useDataRegistry

    • previously this hook just returned a constant instance of DataRegistry, which did not result in the necessary hook => scales => context => component updates when the registry was updated.
    • the new implementation mirrors the DataRegistry API and uses it for its implementation
  • improves the light/dark theme colors:

📝 Documentation

for the /xychart demo:

  • adds checkbox controls to toggle line or bar series
  • adds render horizontally toggle

Adds tests for all new components.

@kristw @techniq @hshoff

horizontal?: boolean;
/**
* Specify bar padding when bar thickness does not come from a `band` scale.
* Accepted values are [0, 1], 0 = no padding, 1 = no bar, defaults to 0.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.

this is backwards and was updated to match d3's band scale logic: 1=no bar, 0=no padding. will fix.

barPadding?: number;
};

function BarSeries<XScale extends AxisScale, YScale extends AxisScale, Datum extends object>({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

most of this logic can become BaseBarSeries in the future to power animated and non-animated bars.

Copy link
Collaborator

@kristw kristw left a comment

Choose a reason for hiding this comment

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

overall lgtm. have small comments and wait for green ci to approve

packages/visx-xychart/src/classes/DataRegistry.ts Outdated Show resolved Hide resolved
@@ -1,55 +1,29 @@
import React, { useContext, useCallback, useEffect } from 'react';
import React, { useContext, useCallback } from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice. The code in LineSeries looks much cleaner with the HOC.

packages/visx-xychart/src/components/series/BarSeries.tsx Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Sep 30, 2020

Pull Request Test Coverage Report for Build 30

Details

  • 66 of 71 (92.96%) changed or added relevant lines in 11 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+1.0%) to 55.334%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/visx-xychart/src/utils/getScaledValueFactory.ts 5 6 83.33%
packages/visx-xychart/src/hooks/useDataRegistry.ts 6 10 60.0%
Totals Coverage Status
Change from base Build 8: 1.0%
Covered Lines: 1156
Relevant Lines: 2030

💛 - Coveralls

const [, forceUpdate] = useState(Math.random());
const privateRegistry = useMemo(() => new DataRegistry<XScale, YScale, Datum>(), []);

const registerData = useCallback(
Copy link
Collaborator

Choose a reason for hiding this comment

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

as all of these 5 functions depends only on privateRegistry, perhaps a single useMemo block is enough?
or should the first two also depend on the value of forceUpdate?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call! initially I didn't have the force update, but you're right that now a single memo works 👌 had to wrap some of the functions for the appropriate this context to work.

@williaster williaster merged commit 5813f1c into master Oct 1, 2020
@williaster williaster deleted the chris--xychart-bar-series branch October 2, 2020 21:58
@williaster williaster added this to the 1.1.0 milestone Oct 17, 2020
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.

3 participants