Skip to content

Commit

Permalink
Refactor the Series and Collection data flow (#302)
Browse files Browse the repository at this point in the history
Simplify a lot of the logic in DataProvider by inverting the source of
truth for Series and Collections. This change moves them from immutable
props on DataProvider to separate children components which report their
configuration (through the DataProvider's Context) into DataProvider's
state. This allows DataProvider to be responsible for watching a lot
fewer props and move the logic out to the separate components.

Additionally, this ensures that the properties for Series and Collection
are centrally-documented, due to them using TypeScript for their
implementations. This also comes with some nice syntactic sugar around
the grouping of Series into Collections (however, this is not required).

Another benefit of this cleaned system is that the system can guarantee
that Series and Collection objects which leave DataProvider are
fully-populated. This will allow us to clean up default props littered
throughout the codebase, for things like strokeWidth and color.

Finally, in order to ease this massively-breaking API change, a thin
backwards-compatibility shim has been introduced into DataProvider.
However, this is not intended to be permanent and should be removed
before the 0.5.0 stable version is released.
  • Loading branch information
Evan Charlton committed May 8, 2019
1 parent 389f603 commit 7ca395e
Show file tree
Hide file tree
Showing 31 changed files with 2,133 additions and 1,838 deletions.
80 changes: 80 additions & 0 deletions src/components/Collection/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import * as React from 'react';
import Data from '../../context/Data';
import { ItemProps, WATCHED_PROP_NAMES, Props as SeriesProps } from '../Series';
import { ItemId } from '../../external';
import { withDisplayName } from '../../utils/displayName';

export interface Props extends ItemProps {
id: ItemId;
}

type UnregisterCollectionFunction = () => void;

type RegisterCollectionFunction = (
collectionProps: Props
) => UnregisterCollectionFunction;

type UpdateCollectionFunction = (collectionProps: Props) => void;

interface InternalProps {
registerCollection: RegisterCollectionFunction;
updateCollection: UpdateCollectionFunction;
children?: React.ReactNode[];
}

// @ts-ignore - I don't know how to make TypeScript happy about ...props
const Collection: React.FunctionComponent<Props & InternalProps> = ({
id,

registerCollection,
updateCollection,
children,

...props
}) => {
React.useEffect(() => {
return registerCollection({
id,
...props,
});
}, []);

React.useEffect(() => {
return updateCollection({
id,
...props,
});
// @ts-ignore - It's okay for props[name] to be implicit any.
}, WATCHED_PROP_NAMES.map(name => props[name]));

if (React.Children.count(children) === 0) {
return null;
}

return React.Children.map(children, child => {
if (!child || !React.isValidElement(child)) {
return null;
}
return React.cloneElement(child as React.ReactElement<SeriesProps>, {
...child.props,
collectionId: id,
});
});
};

export default withDisplayName(
'Collection',
(props: Props & { children: React.ReactNode[] }) => (
<Data.Consumer>
{({ registerCollection, updateCollection }: InternalProps) => (
<Collection
registerCollection={registerCollection}
updateCollection={updateCollection}
{...props}
>
{props.children}
</Collection>
)}
</Data.Consumer>
)
);
10 changes: 6 additions & 4 deletions src/components/ContextChart/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import AxisPlacement from '../AxisPlacement';
import { multiFormat } from '../../utils/multiFormat';
import Axes from '../../utils/Axes';
import { createYScale, createXScale } from '../../utils/scale-helpers';
import { stripPlaceholderDomain } from '../Scaler';
import { firstResolvedDomain } from '../Scaler';
import { calculateDomainFromData } from '../DataProvider';
import { withDisplayName } from '../../utils/displayName';

Expand Down Expand Up @@ -71,7 +71,7 @@ const renderXAxis = (position, xAxis, { xAxisPlacement }) => {
return xAxis;
}
if (xAxisPlacement === AxisPlacement.BOTH) {
return React.cloneElement(xAxis, { xAxisPlacement: position });
return React.cloneElement(xAxis, { placement: position });
}
return null;
};
Expand All @@ -91,8 +91,10 @@ const ContextChart = ({
}) => {
const getYScale = (s, height) => {
const domain =
stripPlaceholderDomain(s.yDomain) ||
stripPlaceholderDomain(Axes.y(domainsByItemId[s.collectionId || s.id])) ||
firstResolvedDomain(
s.yDomain,
Axes.y(domainsByItemId[s.collectionId || s.id])
) ||
calculateDomainFromData(s.data, s.yAccessor, s.y0Accessor, s.y1Accessor);
return createYScale(domain, height);
};
Expand Down

0 comments on commit 7ca395e

Please sign in to comment.