From 9694b5b8a11944d6793782f628c3c192162a660a Mon Sep 17 00:00:00 2001 From: Marco Vettorello Date: Tue, 2 Apr 2019 00:17:22 +0200 Subject: [PATCH] fix(areachart): fix misaligned rendering props (#141) There was a misalignment between the animated props and the non animated ones that causes the area line to use the fill instead of stroke wrongly rendering the line on the chart. We also aligned the animated and not animated props for line and bars. React Spring also distributes ES6 by default, which breaks jest tests unless it's imported explicitly using the cjs extension. see https://github.com/react-spring/react-spring/issues/555 fix #140 --- .../react_canvas/area_geometries.tsx | 142 ++++++------ .../react_canvas/bar_geometries.tsx | 67 +++--- .../react_canvas/line_geometries.tsx | 113 +++++---- .../utils/rendering_props_utils.test.ts | 218 ++++++++++++++++++ .../utils/rendering_props_utils.ts | 181 +++++++++++++++ stories/area_chart.tsx | 8 +- 6 files changed, 555 insertions(+), 174 deletions(-) create mode 100644 src/components/react_canvas/utils/rendering_props_utils.test.ts create mode 100644 src/components/react_canvas/utils/rendering_props_utils.ts diff --git a/src/components/react_canvas/area_geometries.tsx b/src/components/react_canvas/area_geometries.tsx index 41eea5dcbb..6dfcb000bb 100644 --- a/src/components/react_canvas/area_geometries.tsx +++ b/src/components/react_canvas/area_geometries.tsx @@ -1,11 +1,15 @@ import { Group as KonvaGroup } from 'konva'; import React from 'react'; import { Circle, Group, Path } from 'react-konva'; -import { animated, Spring } from 'react-spring/renderprops-konva'; +import { animated, Spring } from 'react-spring/renderprops-konva.cjs'; import { LegendItem } from '../../lib/series/legend'; import { AreaGeometry, getGeometryStyle, PointGeometry } from '../../lib/series/rendering'; import { AreaSeriesStyle, SharedGeometryStyle } from '../../lib/themes/theme'; -import { GlobalKonvaElementProps } from './globals'; +import { + buildAreaLineProps, + buildAreaPointProps, + buildAreaProps, +} from './utils/rendering_props_utils'; interface AreaGeometriesDataProps { animated?: boolean; @@ -54,45 +58,42 @@ export class AreaGeometries extends React.PureComponent< ); } private renderPoints = (areaPoints: PointGeometry[], areaIndex: number): JSX.Element[] => { - const { radius, stroke, strokeWidth, opacity } = this.props.style.point; + const { radius, strokeWidth, opacity } = this.props.style.point; - return areaPoints.map((areaPoint, index) => { + return areaPoints.map((areaPoint, pointIndex) => { const { x, y, color, transform } = areaPoint; if (this.props.animated) { return ( - + - {(props: { y: number }) => ( - - )} + {(props: { y: number }) => { + const pointProps = buildAreaPointProps({ + areaIndex, + pointIndex, + x, + y, + radius, + strokeWidth, + color, + opacity, + }); + return ; + }} ); } else { - return ( - - ); + const pointProps = buildAreaPointProps({ + areaIndex, + pointIndex, + x: transform.x + x, + y, + radius, + strokeWidth, + color, + opacity, + }); + return ; } }); } @@ -108,32 +109,26 @@ export class AreaGeometries extends React.PureComponent< return ( - {(props: { area: string }) => ( - - )} + {(props: { area: string }) => { + const areaProps = buildAreaProps({ + index: i, + areaPath: props.area, + color, + opacity, + }); + return ; + }} ); } else { - return ( - - ); + const areaProps = buildAreaProps({ + index: i, + areaPath: area, + color, + opacity, + }); + return ; } }); } @@ -154,31 +149,28 @@ export class AreaGeometries extends React.PureComponent< return ( - {(props: { line: string }) => ( - - )} + {(props: { line: string }) => { + const lineProps = buildAreaLineProps({ + index: i, + linePath: props.line, + color, + strokeWidth, + geometryStyle, + }); + return ; + }} ); } else { - return ( - - ); + const lineProps = buildAreaLineProps({ + index: i, + linePath: line, + color, + strokeWidth, + geometryStyle, + }); + return ; } }); } diff --git a/src/components/react_canvas/bar_geometries.tsx b/src/components/react_canvas/bar_geometries.tsx index 9b5085a44b..d088ad03f5 100644 --- a/src/components/react_canvas/bar_geometries.tsx +++ b/src/components/react_canvas/bar_geometries.tsx @@ -1,11 +1,11 @@ import { Group as KonvaGroup } from 'konva'; import React from 'react'; import { Group, Rect } from 'react-konva'; -import { animated, Spring } from 'react-spring/renderprops-konva'; +import { animated, Spring } from 'react-spring/renderprops-konva.cjs'; import { LegendItem } from '../../lib/series/legend'; import { BarGeometry, getGeometryStyle } from '../../lib/series/rendering'; import { BarSeriesStyle, SharedGeometryStyle } from '../../lib/themes/theme'; -import { GlobalKonvaElementProps } from './globals'; +import { buildBarProps } from './utils/rendering_props_utils'; interface BarGeometriesDataProps { animated?: boolean; @@ -47,7 +47,7 @@ export class BarGeometries extends React.PureComponent< style: { border }, sharedStyle, } = this.props; - return bars.map((bar, i) => { + return bars.map((bar, index) => { const { x, y, width, height, color } = bar; // Properties to determine if we need to highlight individual bars depending on hover state @@ -69,42 +69,41 @@ export class BarGeometries extends React.PureComponent< const borderEnabled = border.visible && width > border.strokeWidth * 7; if (this.props.animated) { return ( - + - {(props: { y: number; height: number }) => ( - - )} + {(props: { y: number; height: number }) => { + const barProps = buildBarProps({ + index, + x, + y: props.y, + width, + height: props.height, + fill: color, + stroke: border.stroke, + strokeWidth: border.strokeWidth, + borderEnabled, + geometryStyle, + }); + + return ; + }} ); } else { - return ( - - ); + const barProps = buildBarProps({ + index, + x, + y, + width, + height, + fill: color, + stroke: border.stroke, + strokeWidth: border.strokeWidth, + borderEnabled, + geometryStyle, + }); + return ; } }); } diff --git a/src/components/react_canvas/line_geometries.tsx b/src/components/react_canvas/line_geometries.tsx index 7a4ac039ae..62d35438ec 100644 --- a/src/components/react_canvas/line_geometries.tsx +++ b/src/components/react_canvas/line_geometries.tsx @@ -1,11 +1,11 @@ import { Group as KonvaGroup } from 'konva'; import React from 'react'; import { Circle, Group, Path } from 'react-konva'; -import { animated, Spring } from 'react-spring/renderprops-konva'; +import { animated, Spring } from 'react-spring/renderprops-konva.cjs'; import { LegendItem } from '../../lib/series/legend'; import { getGeometryStyle, LineGeometry, PointGeometry } from '../../lib/series/rendering'; import { LineSeriesStyle, SharedGeometryStyle } from '../../lib/themes/theme'; -import { GlobalKonvaElementProps } from './globals'; +import { buildLinePointProps, buildLineProps } from './utils/rendering_props_utils'; interface LineGeometriesDataProps { animated?: boolean; @@ -55,47 +55,43 @@ export class LineGeometries extends React.PureComponent< ); } - private renderPoints = (linePoints: PointGeometry[], i: number): JSX.Element[] => { + private renderPoints = (linePoints: PointGeometry[], lineIndex: number): JSX.Element[] => { const { radius, strokeWidth, opacity } = this.props.style.point; - return linePoints.map((areaPoint, index) => { - const { x, y, color, transform } = areaPoint; + return linePoints.map((linePoint, pointIndex) => { + const { x, y, color, transform } = linePoint; if (this.props.animated) { return ( - + - {(props: { y: number }) => ( - - )} + {(props: { y: number }) => { + const pointProps = buildLinePointProps({ + lineIndex, + pointIndex, + x, + y, + radius, + color, + strokeWidth, + opacity, + }); + return ; + }} ); } else { - return ( - - ); + const pointProps = buildLinePointProps({ + lineIndex, + pointIndex, + x: transform.x + x, + y, + radius, + color, + strokeWidth, + opacity, + }); + return ; } }); } @@ -103,7 +99,7 @@ export class LineGeometries extends React.PureComponent< private renderLineGeoms = (): JSX.Element[] => { const { style, lines, sharedStyle } = this.props; const { strokeWidth } = style.line; - return lines.map((glyph, i) => { + return lines.map((glyph, index) => { const { line, color, transform, geometryId } = glyph; const geometryStyle = getGeometryStyle( @@ -114,37 +110,32 @@ export class LineGeometries extends React.PureComponent< if (this.props.animated) { return ( - + - {(props: { opacity: number }) => ( - - )} + {(props: { opacity: number }) => { + const lineProps = buildLineProps({ + index, + linePath: line, + color, + strokeWidth, + opacity: props.opacity, + geometryStyle, + }); + return ; + }} ); } else { - return ( - - ); + const lineProps = buildLineProps({ + index, + linePath: line, + color, + strokeWidth, + opacity: 1, + geometryStyle, + }); + return ; } }); } diff --git a/src/components/react_canvas/utils/rendering_props_utils.test.ts b/src/components/react_canvas/utils/rendering_props_utils.test.ts new file mode 100644 index 0000000000..1829601a5f --- /dev/null +++ b/src/components/react_canvas/utils/rendering_props_utils.test.ts @@ -0,0 +1,218 @@ +import { + buildAreaLineProps, + buildAreaPointProps, + buildAreaProps, + buildBarProps, + buildLinePointProps, + buildLineProps, +} from './rendering_props_utils'; + +describe('[canvas] Area Geometries props', () => { + test('can build area point props', () => { + const props = buildAreaPointProps({ + areaIndex: 1, + pointIndex: 2, + x: 10, + y: 20, + radius: 30, + strokeWidth: 2, + color: 'red', + opacity: 0.5, + }); + expect(props).toEqual({ + key: 'area-point-1-2', + x: 10, + y: 20, + radius: 30, + strokeWidth: 2, + strokeEnabled: true, + stroke: 'red', + fill: 'white', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + + const propsNoStroke = buildAreaPointProps({ + areaIndex: 1, + pointIndex: 2, + x: 10, + y: 20, + radius: 30, + strokeWidth: 0, + color: 'red', + opacity: 0.5, + }); + expect(propsNoStroke).toEqual({ + key: 'area-point-1-2', + x: 10, + y: 20, + radius: 30, + strokeWidth: 0, + strokeEnabled: false, + stroke: 'red', + fill: 'white', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + }); + test('can build area path props', () => { + const props = buildAreaProps({ + index: 1, + areaPath: 'M0,0L10,10Z', + color: 'red', + opacity: 0.5, + }); + expect(props).toEqual({ + key: 'area-1', + data: 'M0,0L10,10Z', + fill: 'red', + lineCap: 'round', + lineJoin: 'round', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + }); + test('can build area line path props', () => { + const props = buildAreaLineProps({ + index: 1, + linePath: 'M0,0L10,10Z', + color: 'red', + strokeWidth: 1, + geometryStyle: { + opacity: 0.5, + }, + }); + expect(props).toEqual({ + key: `area-line-1`, + data: 'M0,0L10,10Z', + stroke: 'red', + strokeWidth: 1, + lineCap: 'round', + lineJoin: 'round', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + expect(props.fill).toBeFalsy(); + }); +}); + +describe('[canvas] Line Geometries', () => { + test('can build line point props', () => { + const props = buildLinePointProps({ + lineIndex: 1, + pointIndex: 2, + x: 10, + y: 20, + radius: 30, + strokeWidth: 2, + color: 'red', + opacity: 0.5, + }); + expect(props).toEqual({ + key: 'line-point-1-2', + x: 10, + y: 20, + radius: 30, + strokeWidth: 2, + strokeEnabled: true, + stroke: 'red', + fill: 'white', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + + const propsNoStroke = buildLinePointProps({ + lineIndex: 1, + pointIndex: 2, + x: 10, + y: 20, + radius: 30, + strokeWidth: 0, + color: 'red', + opacity: 0.5, + }); + expect(propsNoStroke).toEqual({ + key: 'line-point-1-2', + x: 10, + y: 20, + radius: 30, + strokeWidth: 0, + strokeEnabled: false, + stroke: 'red', + fill: 'white', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + }); + test('can build line path props', () => { + const props = buildLineProps({ + index: 1, + linePath: 'M0,0L10,10Z', + color: 'red', + strokeWidth: 1, + opacity: 0.3, + geometryStyle: { + opacity: 0.5, + }, + }); + expect(props).toEqual({ + key: `line-1`, + data: 'M0,0L10,10Z', + stroke: 'red', + strokeWidth: 1, + lineCap: 'round', + lineJoin: 'round', + opacity: 0.5, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + }); + expect(props.fill).toBeFalsy(); + }); +}); + +describe('[canvas] Bar Geometries', () => { + test('can build bar props', () => { + const props = buildBarProps({ + index: 1, + x: 10, + y: 20, + width: 30, + height: 40, + fill: 'red', + stroke: 'blue', + strokeWidth: 1, + borderEnabled: true, + geometryStyle: { + opacity: 0.5, + }, + }); + expect(props).toEqual({ + key: `bar-1`, + x: 10, + y: 20, + width: 30, + height: 40, + fill: 'red', + stroke: 'blue', + strokeWidth: 1, + strokeEnabled: true, + strokeHitEnabled: false, + perfectDrawEnabled: false, + listening: false, + opacity: 0.5, + }); + }); +}); diff --git a/src/components/react_canvas/utils/rendering_props_utils.ts b/src/components/react_canvas/utils/rendering_props_utils.ts new file mode 100644 index 0000000000..4cc23101a9 --- /dev/null +++ b/src/components/react_canvas/utils/rendering_props_utils.ts @@ -0,0 +1,181 @@ +import { GeometryStyle } from '../../../lib/series/rendering'; +import { GlobalKonvaElementProps } from '../globals'; + +export function buildAreaPointProps({ + areaIndex, + pointIndex, + x, + y, + radius, + strokeWidth, + color, + opacity, +}: { + areaIndex: number; + pointIndex: number; + x: number; + y: number; + radius: number; + strokeWidth: number; + color: string; + opacity: number; +}) { + return { + key: `area-point-${areaIndex}-${pointIndex}`, + x, + y, + radius, + strokeWidth, + strokeEnabled: strokeWidth !== 0, + stroke: color, + fill: 'white', + opacity, + ...GlobalKonvaElementProps, + }; +} + +export function buildAreaProps({ + index, + areaPath, + color, + opacity, +}: { + index: number; + areaPath: string; + color: string; + opacity: number; +}) { + return { + key: `area-${index}`, + data: areaPath, + fill: color, + lineCap: 'round', + lineJoin: 'round', + opacity, + ...GlobalKonvaElementProps, + }; +} + +export function buildAreaLineProps({ + index, + linePath, + color, + strokeWidth, + geometryStyle, +}: { + index: number; + linePath: string; + color: string; + strokeWidth: number; + geometryStyle: GeometryStyle; +}) { + return { + key: `area-line-${index}`, + data: linePath, + stroke: color, + strokeWidth, + lineCap: 'round', + lineJoin: 'round', + ...geometryStyle, + ...GlobalKonvaElementProps, + }; +} + +export function buildBarProps({ + index, + x, + y, + width, + height, + fill, + stroke, + strokeWidth, + borderEnabled, + geometryStyle, +}: { + index: number; + x: number; + y: number; + width: number; + height: number; + fill: string; + stroke: string; + strokeWidth: number; + borderEnabled: boolean; + geometryStyle: GeometryStyle; +}) { + return { + key: `bar-${index}`, + x, + y, + width, + height, + fill, + strokeWidth, + stroke, + strokeEnabled: borderEnabled, + ...GlobalKonvaElementProps, + ...geometryStyle, + }; +} + +export function buildLinePointProps({ + lineIndex, + pointIndex, + x, + y, + radius, + strokeWidth, + color, + opacity, +}: { + lineIndex: number; + pointIndex: number; + x: number; + y: number; + radius: number; + strokeWidth: number; + color: string; + opacity: number; +}) { + return { + key: `line-point-${lineIndex}-${pointIndex}`, + x, + y, + radius, + stroke: color, + strokeWidth, + strokeEnabled: strokeWidth !== 0, + fill: 'white', + opacity, + ...GlobalKonvaElementProps, + }; +} + +export function buildLineProps({ + index, + linePath, + color, + strokeWidth, + opacity, + geometryStyle, +}: { + index: number; + linePath: string; + color: string; + strokeWidth: number; + opacity: number; + geometryStyle: GeometryStyle; +}) { + return { + key: `line-${index}`, + data: linePath, + stroke: color, + strokeWidth, + opacity, + lineCap: 'round', + lineJoin: 'round', + ...geometryStyle, + ...GlobalKonvaElementProps, + }; +} diff --git a/stories/area_chart.tsx b/stories/area_chart.tsx index 91f95727fc..1f6794a834 100644 --- a/stories/area_chart.tsx +++ b/stories/area_chart.tsx @@ -21,7 +21,7 @@ storiesOf('Area Chart', module) return ( Number(d).toFixed(2)} />