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.

Sign up for GitHub

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

fix(rendering): area, line and point rendered on the same layer for each series #290

Merged
merged 4 commits into from
Aug 13, 2019
Merged

fix(rendering): area, line and point rendered on the same layer for each series #290

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
17458ea
fix(lines): line and point rendered on the same layer for each series
markov00 Aug 6, 2019
f612276
refactor(areas): fix areas rendering order
markov00 Aug 9, 2019
5d712a5
refactor(line): fix line keys and remove springs
markov00 Aug 9, 2019
c4fffd9
refactor: rename getGeometryId _AsString to _Key
markov00 Aug 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 38 additions & 43 deletions src/components/react_canvas/line_geometries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,27 +38,10 @@ export class LineGeometries extends React.PureComponent<LineGeometriesDataProps,
return (
<Group ref={this.barSeriesRef} key={'bar_series'}>
{this.renderLineGeoms()}
{this.renderLinePoints()}
</Group>
);
}

private renderLinePoints = (): JSX.Element[] => {
const { lines } = this.props;
return lines.reduce(
(acc, glyph, i) => {
const { points, seriesPointStyle, color } = glyph;

if (!seriesPointStyle.visible) {
return acc;
}
const pointStyleProps = buildPointStyleProps(color, seriesPointStyle);
return [...acc, ...this.renderPoints(points, i, pointStyleProps)];
},
[] as JSX.Element[],
);
};

private renderPoints = (
linePoints: PointGeometry[],
lineIndex: number,
Expand Down Expand Up @@ -90,35 +73,47 @@ export class LineGeometries extends React.PureComponent<LineGeometriesDataProps,
private renderLineGeoms = (): JSX.Element[] => {
const { lines, sharedStyle } = this.props;

const lineElements: JSX.Element[] = [];

lines.forEach((glyph, index) => {
const { line, color, transform, geometryId, seriesLineStyle } = glyph;
return lines.reduce<JSX.Element[]>((acc, glyph, index) => {
const { seriesLineStyle, seriesPointStyle } = glyph;

if (!seriesLineStyle.visible) {
return;
if (seriesLineStyle.visible) {
acc.push(this.getLineToRender(glyph, sharedStyle, index));
}
const key = `line-${index}`;
const customOpacity = seriesLineStyle ? seriesLineStyle.opacity : undefined;
const geometryStyle = getGeometryStyle(geometryId, this.props.highlightedLegendItem, sharedStyle, customOpacity);

if (this.props.animated) {
lineElements.push(
<Group key={index} x={transform.x}>
<Spring native reset from={{ opacity: 0 }} to={{ opacity: 1 }}>
{() => {
const lineProps = buildLineRenderProps(0, line, color, seriesLineStyle, geometryStyle);
return <animated.Path {...lineProps} key={key} />;
}}
</Spring>
</Group>,
);
} else {
const lineProps = buildLineRenderProps(transform.x, line, color, seriesLineStyle, geometryStyle);
lineElements.push(<Path {...lineProps} key={key} />);
if (seriesPointStyle.visible) {
acc.push(...this.getPointToRender(glyph, index));
}
});

return lineElements;
return acc;
}, []);
};

getLineToRender(glyph: LineGeometry, sharedStyle: SharedGeometryStyle, index: number) {
const { line, color, transform, geometryId, seriesLineStyle } = glyph;
const key = `line-${index}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use something other than the index for the key?

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on 5d712a5

const customOpacity = seriesLineStyle ? seriesLineStyle.opacity : undefined;
const geometryStyle = getGeometryStyle(geometryId, this.props.highlightedLegendItem, sharedStyle, customOpacity);

if (this.props.animated) {
return (
<Group key={index} x={transform.x}>
Copy link
Collaborator

@nickofthyme nickofthyme Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Also here? re: index

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on 5d712a5

<Spring native reset from={{ opacity: 0 }} to={{ opacity: 1 }}>
{() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we extract this function so it doesn't create a new function on each rerender? I don't think it will be that different on performance but best to keep up with best practices.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed the animation in 5d712a5

const lineProps = buildLineRenderProps(0, line, color, seriesLineStyle, geometryStyle);
return <animated.Path {...lineProps} key={key} />;
}}
</Spring>
</Group>
);
} else {
const lineProps = buildLineRenderProps(transform.x, line, color, seriesLineStyle, geometryStyle);
return <Path {...lineProps} key={key} />;
}
}

getPointToRender(glyph: LineGeometry, index: number) {
const { points, color, seriesPointStyle } = glyph;

const pointStyleProps = buildPointStyleProps(color, seriesPointStyle);
return this.renderPoints(points, index, pointStyleProps);
}
}