From 37808fff9299cb964f645edb0e759d21cad1ab85 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Tue, 10 Sep 2019 17:31:00 -0500 Subject: [PATCH 1/5] feat(axis): option to hide duplicate axes * Option to hide axes based on tick labels, position and title. * Refactor axes render function. closes #368 --- src/chart_types/xy_chart/store/chart_state.ts | 37 +++++++- .../react_canvas/reactive_chart.tsx | 90 +++++++++++-------- src/specs/settings.test.tsx | 2 + src/specs/settings.tsx | 9 ++ stories/scales.tsx | 38 +++++++- 5 files changed, 138 insertions(+), 38 deletions(-) diff --git a/src/chart_types/xy_chart/store/chart_state.ts b/src/chart_types/xy_chart/store/chart_state.ts index 7f16cd4c40..06e721a363 100644 --- a/src/chart_types/xy_chart/store/chart_state.ts +++ b/src/chart_types/xy_chart/store/chart_state.ts @@ -155,6 +155,7 @@ export class ChartStore { chartRotation: Rotation = 0; // updated from jsx chartRendering: Rendering = 'canvas'; // updated from jsx chartTheme: Theme = LIGHT_THEME; + hideDuplicateAxes: boolean = false; // updated from jsx axesSpecs: Map = new Map(); // readed from jsx axesTicksDimensions: Map = new Map(); // computed axesPositions: Map = new Map(); // computed @@ -844,6 +845,35 @@ export class ChartStore { this.annotationSpecs.delete(annotationId); } + isDuplicateAxis( + { position, title }: AxisSpec, + { tickLabels }: AxisTicksDimensions, + tickMap: Map, + specMap: Map, + ): boolean { + const firstTickLabel = tickLabels[0]; + const lastTickLabel = tickLabels.slice(-1)[0]; + + let hasDuplicate = false; + tickMap.forEach(({ tickLabels: axisTickLabels }, axisId) => { + if ( + !hasDuplicate && + axisTickLabels && + tickLabels.length === axisTickLabels.length && + firstTickLabel === axisTickLabels[0] && + lastTickLabel === axisTickLabels.slice(-1)[0] + ) { + const spec = specMap.get(axisId); + + if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) { + hasDuplicate = true; + } + } + }); + + return hasDuplicate; + } + computeChart() { this.chartInitialized.set(false); // compute only if parent dimensions are computed @@ -926,7 +956,12 @@ export class ChartStore { barsPadding, this.enableHistogramMode.get(), ); - if (dimensions) { + + if ( + dimensions && + (!this.hideDuplicateAxes || + !this.isDuplicateAxis(axisSpec, dimensions, this.axesTicksDimensions, this.axesSpecs)) + ) { this.axesTicksDimensions.set(id, dimensions); } }); diff --git a/src/components/react_canvas/reactive_chart.tsx b/src/components/react_canvas/reactive_chart.tsx index 5683fbbf1b..7492c308ab 100644 --- a/src/components/react_canvas/reactive_chart.tsx +++ b/src/components/react_canvas/reactive_chart.tsx @@ -1,9 +1,9 @@ import { inject, observer } from 'mobx-react'; import React from 'react'; import { Layer, Rect, Stage } from 'react-konva'; -import { isLineAnnotation, isRectAnnotation } from '../../chart_types/xy_chart/utils/specs'; +import { isLineAnnotation, isRectAnnotation, AxisSpec } from '../../chart_types/xy_chart/utils/specs'; import { LineAnnotationStyle, RectAnnotationStyle } from '../../utils/themes/theme'; -import { AnnotationId } from '../../utils/ids'; +import { AnnotationId, AxisId } from '../../utils/ids'; import { AnnotationDimensions, AnnotationLineProps, @@ -20,6 +20,8 @@ import { LineAnnotation } from './line_annotation'; import { LineGeometries } from './line_geometries'; import { RectAnnotation } from './rect_annotation'; import { ContainerConfig } from 'konva'; +import { AxisTick, AxisTicksDimensions } from '../../chart_types/xy_chart/utils/axis_utils'; +import { Dimensions } from '../../utils/dimensions'; interface ReactiveChartProps { chartStore?: ChartStore; // FIX until we find a better way on ts mobx @@ -34,6 +36,14 @@ interface ReactiveChartState { }; } +interface AxisConfig { + id: AxisId; + axisSpec: AxisSpec; + axisTicksDimensions: AxisTicksDimensions; + axisPosition: Dimensions; + ticks: AxisTick[]; +} + interface ReactiveChartElementIndex { element: JSX.Element; zIndex: number; @@ -151,40 +161,50 @@ class Chart extends React.Component { }, ]; }; - renderAxes = () => { - const { - axesVisibleTicks, - axesSpecs, - axesTicksDimensions, - axesPositions, - chartTheme, - debug, - chartDimensions, - } = this.props.chartStore!; - const axesComponents: JSX.Element[] = []; - axesVisibleTicks.forEach((axisTicks, axisId) => { - const axisSpec = axesSpecs.get(axisId); - const axisTicksDimensions = axesTicksDimensions.get(axisId); - const axisPosition = axesPositions.get(axisId); - const ticks = axesVisibleTicks.get(axisId); - if (!ticks || !axisSpec || !axisTicksDimensions || !axisPosition) { - return; - } - axesComponents.push( - , - ); - }); - return axesComponents; + getAxes = (): AxisConfig[] => { + const { axesVisibleTicks, axesSpecs, axesTicksDimensions, axesPositions } = this.props.chartStore!; + const ids = [...axesVisibleTicks.keys()]; + + return ids.reduce( + (acc, id) => { + const ticks = axesVisibleTicks.get(id); + const axisSpec = axesSpecs.get(id); + const axisTicksDimensions = axesTicksDimensions.get(id); + const axisPosition = axesPositions.get(id); + + if (ticks && axisSpec && axisTicksDimensions && axisPosition) { + acc.push({ + id, + ticks, + axisSpec, + axisTicksDimensions, + axisPosition, + }); + } + + return acc; + }, + [] as AxisConfig[], + ); + }; + + renderAxes = (): JSX.Element[] => { + const { chartTheme, debug, chartDimensions } = this.props.chartStore!; + const axes = this.getAxes(); + + return axes.map(({ id, ticks, axisSpec, axisTicksDimensions, axisPosition }) => ( + + )); }; renderGrids = () => { diff --git a/src/specs/settings.test.tsx b/src/specs/settings.test.tsx index 70b13915f3..caa6cde14c 100644 --- a/src/specs/settings.test.tsx +++ b/src/specs/settings.test.tsx @@ -79,6 +79,7 @@ describe('Settings spec component', () => { snap: false, }, legendPosition: Position.Bottom, + hideDuplicateAxes: false, showLegendDisplayValue: false, debug: true, xDomain: { min: 0, max: 10 }, @@ -183,6 +184,7 @@ describe('Settings spec component', () => { }, legendPosition: Position.Bottom, showLegendDisplayValue: false, + hideDuplicateAxes: false, debug: true, xDomain: { min: 0, max: 10 }, }; diff --git a/src/specs/settings.tsx b/src/specs/settings.tsx index 1c45e08fb3..e3e9f1ab41 100644 --- a/src/specs/settings.tsx +++ b/src/specs/settings.tsx @@ -78,6 +78,12 @@ export interface SettingSpecProps { debug: boolean; legendPosition?: Position; showLegendDisplayValue: boolean; + /** + * Removes duplicate axes + * + * Compares title, position and first & last tick labels + */ + hideDuplicateAxes: boolean; onElementClick?: ElementClickListener; onElementOver?: ElementOverListener; onElementOut?: () => undefined | void; @@ -130,6 +136,7 @@ function updateChartStore(props: SettingSpecProps) { debug, xDomain, resizeDebounce, + hideDuplicateAxes, } = props; if (!chartStore) { @@ -142,6 +149,7 @@ function updateChartStore(props: SettingSpecProps) { chartStore.animateData = animateData; chartStore.debug = debug; chartStore.resizeDebounce = resizeDebounce!; + chartStore.hideDuplicateAxes = hideDuplicateAxes; if (tooltip && isTooltipProps(tooltip)) { const { type, snap, headerFormatter } = tooltip; @@ -203,6 +211,7 @@ export class SettingsComponent extends PureComponent { showLegend: false, resizeDebounce: 10, debug: false, + hideDuplicateAxes: false, tooltip: { type: DEFAULT_TOOLTIP_TYPE, snap: DEFAULT_TOOLTIP_SNAP, diff --git a/stories/scales.tsx b/stories/scales.tsx index d5703d7903..28d8adfdd2 100644 --- a/stories/scales.tsx +++ b/stories/scales.tsx @@ -1,8 +1,8 @@ -import { select } from '@storybook/addon-knobs'; +import { select, boolean } from '@storybook/addon-knobs'; import { storiesOf } from '@storybook/react'; import { DateTime } from 'luxon'; import React from 'react'; -import { Axis, Chart, getAxisId, getSpecId, LineSeries, Position, ScaleType } from '../src'; +import { Axis, Chart, getAxisId, getSpecId, LineSeries, Position, ScaleType, Settings } from '../src'; const today = new Date().getTime(); const UTC_DATE = DateTime.fromISO('2019-01-01T00:00:00.000Z').toMillis(); @@ -201,4 +201,38 @@ storiesOf('Scales', module) remember to apply the same timezone also to each formatted tick in \`tickFormat\` `, }, }, + ) + .add( + 'Remove duplicate scales', + () => { + return ( + + + + `${d}%`} /> + `${d}%`} /> + `${d}%`} + /> + `${d}%`} /> + + + ); + }, + { + info: { + text: '`hideDuplicateAxes` will remove redundant axes that have the same min and max labels and position', + }, + }, ); From f306a9366b3019ff27384925bec4bbd2796b3898 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 12 Sep 2019 09:40:27 -0500 Subject: [PATCH 2/5] fix: pr comments --- src/chart_types/xy_chart/store/chart_state.ts | 61 +++++++++---------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/src/chart_types/xy_chart/store/chart_state.ts b/src/chart_types/xy_chart/store/chart_state.ts index 06e721a363..08d42466f3 100644 --- a/src/chart_types/xy_chart/store/chart_state.ts +++ b/src/chart_types/xy_chart/store/chart_state.ts @@ -114,6 +114,35 @@ export type CursorUpdateListener = (event?: CursorEvent) => void; export type RenderChangeListener = (isRendered: boolean) => void; export type BasicListener = () => undefined | void; +export const isDuplicateAxis = ( + { position, title }: AxisSpec, + { tickLabels }: AxisTicksDimensions, + tickMap: Map, + specMap: Map, +): boolean => { + const firstTickLabel = tickLabels[0]; + const lastTickLabel = tickLabels.slice(-1)[0]; + + let hasDuplicate = false; + tickMap.forEach(({ tickLabels: axisTickLabels }, axisId) => { + if ( + !hasDuplicate && + axisTickLabels && + tickLabels.length === axisTickLabels.length && + firstTickLabel === axisTickLabels[0] && + lastTickLabel === axisTickLabels.slice(-1)[0] + ) { + const spec = specMap.get(axisId); + + if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) { + hasDuplicate = true; + } + } + }); + + return hasDuplicate; +}; + export class ChartStore { constructor(id?: string) { this.id = id || uuid.v4(); @@ -845,35 +874,6 @@ export class ChartStore { this.annotationSpecs.delete(annotationId); } - isDuplicateAxis( - { position, title }: AxisSpec, - { tickLabels }: AxisTicksDimensions, - tickMap: Map, - specMap: Map, - ): boolean { - const firstTickLabel = tickLabels[0]; - const lastTickLabel = tickLabels.slice(-1)[0]; - - let hasDuplicate = false; - tickMap.forEach(({ tickLabels: axisTickLabels }, axisId) => { - if ( - !hasDuplicate && - axisTickLabels && - tickLabels.length === axisTickLabels.length && - firstTickLabel === axisTickLabels[0] && - lastTickLabel === axisTickLabels.slice(-1)[0] - ) { - const spec = specMap.get(axisId); - - if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) { - hasDuplicate = true; - } - } - }); - - return hasDuplicate; - } - computeChart() { this.chartInitialized.set(false); // compute only if parent dimensions are computed @@ -959,8 +959,7 @@ export class ChartStore { if ( dimensions && - (!this.hideDuplicateAxes || - !this.isDuplicateAxis(axisSpec, dimensions, this.axesTicksDimensions, this.axesSpecs)) + (!this.hideDuplicateAxes || !isDuplicateAxis(axisSpec, dimensions, this.axesTicksDimensions, this.axesSpecs)) ) { this.axesTicksDimensions.set(id, dimensions); } From add966123fc1cacd50f5caa2ee8cc854abff5e32 Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 12 Sep 2019 09:45:47 -0500 Subject: [PATCH 3/5] test(chart_state): add tests for isDiplicateAxis function --- .../xy_chart/store/chart_state.test.ts | 147 +++++++++++++++++- 1 file changed, 145 insertions(+), 2 deletions(-) diff --git a/src/chart_types/xy_chart/store/chart_state.test.ts b/src/chart_types/xy_chart/store/chart_state.test.ts index c37d7a0021..5d3e1ea730 100644 --- a/src/chart_types/xy_chart/store/chart_state.test.ts +++ b/src/chart_types/xy_chart/store/chart_state.test.ts @@ -11,12 +11,13 @@ import { } from '../utils/specs'; import { LIGHT_THEME } from '../../../utils/themes/light_theme'; import { mergeWithDefaultTheme } from '../../../utils/themes/theme'; -import { getAnnotationId, getAxisId, getGroupId, getSpecId } from '../../../utils/ids'; +import { getAnnotationId, getAxisId, getGroupId, getSpecId, AxisId } from '../../../utils/ids'; import { TooltipType, TooltipValue } from '../utils/interactions'; import { ScaleBand } from '../../../utils/scales/scale_band'; import { ScaleContinuous } from '../../../utils/scales/scale_continuous'; import { ScaleType } from '../../../utils/scales/scales'; -import { ChartStore } from './chart_state'; +import { ChartStore, isDuplicateAxis } from './chart_state'; +import { AxisTicksDimensions } from '../utils/axis_utils'; describe('Chart Store', () => { let store = new ChartStore(); @@ -71,6 +72,148 @@ describe('Chart Store', () => { store.computeChart(); }); + describe('isDuplicateAxis', () => { + const AXIS_1_ID = getAxisId('spec_1'); + const AXIS_2_ID = getAxisId('spec_1'); + const axis1: AxisSpec = { + id: AXIS_1_ID, + groupId: getGroupId('group_1'), + hide: false, + showOverlappingTicks: false, + showOverlappingLabels: false, + position: Position.Left, + tickSize: 30, + tickPadding: 10, + tickFormat: (value: any) => `${value}%`, + }; + const axis2: AxisSpec = { + ...axis1, + id: AXIS_2_ID, + groupId: getGroupId('group_2'), + }; + const axisTicksDimensions: AxisTicksDimensions = { + tickValues: [], + tickLabels: ['10', '20', '30'], + maxLabelBboxWidth: 1, + maxLabelBboxHeight: 1, + maxLabelTextWidth: 1, + maxLabelTextHeight: 1, + }; + let tickMap: Map; + let specMap: Map; + + beforeEach(() => { + tickMap = new Map(); + specMap = new Map(); + }); + + it('should return true if axisSpecs and ticks match', () => { + tickMap.set(AXIS_2_ID, axisTicksDimensions); + specMap.set(AXIS_2_ID, axis2); + const result = isDuplicateAxis(axis1, axisTicksDimensions, tickMap, specMap); + + expect(result).toBe(true); + }); + + it('should return false if axisSpecs, ticks AND title match', () => { + tickMap.set(AXIS_2_ID, axisTicksDimensions); + specMap.set(AXIS_2_ID, { + ...axis2, + title: 'TESTING', + }); + const result = isDuplicateAxis( + { + ...axis1, + title: 'TESTING', + }, + axisTicksDimensions, + tickMap, + specMap, + ); + + expect(result).toBe(true); + }); + + it('should return true with single tick', () => { + const newAxisTicksDimensions = { + ...axisTicksDimensions, + tickLabels: ['10'], + }; + tickMap.set(AXIS_2_ID, newAxisTicksDimensions); + specMap.set(AXIS_2_ID, axis2); + + const result = isDuplicateAxis(axis1, newAxisTicksDimensions, tickMap, specMap); + + expect(result).toBe(true); + }); + + it('should return false if axisSpecs and ticks match but title is different', () => { + tickMap.set(AXIS_2_ID, axisTicksDimensions); + specMap.set(AXIS_2_ID, { + ...axis2, + title: 'TESTING', + }); + const result = isDuplicateAxis( + { + ...axis1, + title: 'NOT TESTING', + }, + axisTicksDimensions, + tickMap, + specMap, + ); + + expect(result).toBe(false); + }); + + it('should return false if axisSpecs and ticks match but position is different', () => { + tickMap.set(AXIS_2_ID, axisTicksDimensions); + specMap.set(AXIS_2_ID, axis2); + const result = isDuplicateAxis( + { + ...axis1, + position: Position.Top, + }, + axisTicksDimensions, + tickMap, + specMap, + ); + + expect(result).toBe(false); + }); + + it('should return false if tickFormat is different', () => { + tickMap.set(AXIS_2_ID, { + ...axisTicksDimensions, + tickLabels: ['10%', '20%', '30%'], + }); + specMap.set(AXIS_2_ID, axis2); + + const result = isDuplicateAxis(axis1, axisTicksDimensions, tickMap, specMap); + + expect(result).toBe(false); + }); + + it('should return false if tick label count is different', () => { + tickMap.set(AXIS_2_ID, { + ...axisTicksDimensions, + tickLabels: ['10', '20', '25', '30'], + }); + specMap.set(AXIS_2_ID, axis2); + + const result = isDuplicateAxis(axis1, axisTicksDimensions, tickMap, specMap); + + expect(result).toBe(false); + }); + + it("should return false if can't find spec", () => { + tickMap.set(AXIS_2_ID, axisTicksDimensions); + const result = isDuplicateAxis(axis1, axisTicksDimensions, tickMap, specMap); + + expect(result).toBe(false); + }); + }); + test('can add a single spec', () => { store.addSeriesSpec(spec); store.updateParentDimensions(600, 600, 0, 0); From 37c4f0b70ed2e2d4cb2d4709d6f679e109e227ec Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Thu, 12 Sep 2019 12:10:45 -0500 Subject: [PATCH 4/5] refactor: update reduce to map.filter --- .../react_canvas/reactive_chart.tsx | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/components/react_canvas/reactive_chart.tsx b/src/components/react_canvas/reactive_chart.tsx index b19aebe3c5..8a27f3401a 100644 --- a/src/components/react_canvas/reactive_chart.tsx +++ b/src/components/react_canvas/reactive_chart.tsx @@ -167,27 +167,21 @@ class Chart extends React.Component { const { axesVisibleTicks, axesSpecs, axesTicksDimensions, axesPositions } = this.props.chartStore!; const ids = [...axesVisibleTicks.keys()]; - return ids.reduce( - (acc, id) => { - const ticks = axesVisibleTicks.get(id); - const axisSpec = axesSpecs.get(id); - const axisTicksDimensions = axesTicksDimensions.get(id); - const axisPosition = axesPositions.get(id); - - if (ticks && axisSpec && axisTicksDimensions && axisPosition) { - acc.push({ - id, - ticks, - axisSpec, - axisTicksDimensions, - axisPosition, - }); - } - - return acc; - }, - [] as AxisConfig[], - ); + return ids + .map((id) => ({ + id, + ticks: axesVisibleTicks.get(id), + axisSpec: axesSpecs.get(id), + axisTicksDimensions: axesTicksDimensions.get(id), + axisPosition: axesPositions.get(id), + })) + .filter( + (config: Partial): config is AxisConfig => { + const { ticks, axisSpec, axisTicksDimensions, axisPosition } = config; + + return Boolean(ticks && axisSpec && axisTicksDimensions && axisPosition); + }, + ); }; renderAxes = (): JSX.Element[] => { From 24747769f4a2261c46ae71a3e1502c402cd6d45b Mon Sep 17 00:00:00 2001 From: nickofthyme Date: Mon, 16 Sep 2019 14:37:45 -0500 Subject: [PATCH 5/5] fix: pr comments --- src/chart_types/xy_chart/store/chart_state.ts | 2 +- .../react_canvas/reactive_chart.tsx | 25 ++++++------------- 2 files changed, 9 insertions(+), 18 deletions(-) diff --git a/src/chart_types/xy_chart/store/chart_state.ts b/src/chart_types/xy_chart/store/chart_state.ts index 08d42466f3..e03958feb2 100644 --- a/src/chart_types/xy_chart/store/chart_state.ts +++ b/src/chart_types/xy_chart/store/chart_state.ts @@ -134,7 +134,7 @@ export const isDuplicateAxis = ( ) { const spec = specMap.get(axisId); - if (spec && spec.position === position && ((!title && !spec.title) || title === spec.title)) { + if (spec && spec.position === position && title === spec.title) { hasDuplicate = true; } } diff --git a/src/components/react_canvas/reactive_chart.tsx b/src/components/react_canvas/reactive_chart.tsx index 4ec800505b..2fe3582caf 100644 --- a/src/components/react_canvas/reactive_chart.tsx +++ b/src/components/react_canvas/reactive_chart.tsx @@ -3,7 +3,7 @@ import { inject, observer } from 'mobx-react'; import { ContainerConfig } from 'konva'; import { Layer, Rect, Stage } from 'react-konva'; -import { AnnotationId, AxisId } from '../../utils/ids'; +import { AnnotationId } from '../../utils/ids'; import { isLineAnnotation, isRectAnnotation, AxisSpec } from '../../chart_types/xy_chart/utils/specs'; import { LineAnnotationStyle, RectAnnotationStyle, mergeGridLineConfigs } from '../../utils/themes/theme'; import { @@ -37,8 +37,8 @@ interface ReactiveChartState { }; } -interface AxisConfig { - id: AxisId; +interface AxisProps { + key: string; axisSpec: AxisSpec; axisTicksDimensions: AxisTicksDimensions; axisPosition: Dimensions; @@ -163,20 +163,20 @@ class Chart extends React.Component { ]; }; - getAxes = (): AxisConfig[] => { + getAxes = (): AxisProps[] => { const { axesVisibleTicks, axesSpecs, axesTicksDimensions, axesPositions } = this.props.chartStore!; const ids = [...axesVisibleTicks.keys()]; return ids .map((id) => ({ - id, + key: `axis-${id}`, ticks: axesVisibleTicks.get(id), axisSpec: axesSpecs.get(id), axisTicksDimensions: axesTicksDimensions.get(id), axisPosition: axesPositions.get(id), })) .filter( - (config: Partial): config is AxisConfig => { + (config: Partial): config is AxisProps => { const { ticks, axisSpec, axisTicksDimensions, axisPosition } = config; return Boolean(ticks && axisSpec && axisTicksDimensions && axisPosition); @@ -188,17 +188,8 @@ class Chart extends React.Component { const { chartTheme, debug, chartDimensions } = this.props.chartStore!; const axes = this.getAxes(); - return axes.map(({ id, ticks, axisSpec, axisTicksDimensions, axisPosition }) => ( - + return axes.map(({ key, ...axisProps }) => ( + )); };