From 473b558eb974e1900d67c873210f9a2d8ea8d600 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 13:56:11 -0400 Subject: [PATCH 01/10] Add fix --- charts/core/src/Chart/hooks/updateUtils.ts | 6 +++--- charts/core/src/Chart/hooks/useChart.ts | 7 ++++++- charts/core/src/Line/Line.tsx | 10 +++++++++- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/charts/core/src/Chart/hooks/updateUtils.ts b/charts/core/src/Chart/hooks/updateUtils.ts index 829535ddcf..fa4aa98b3b 100644 --- a/charts/core/src/Chart/hooks/updateUtils.ts +++ b/charts/core/src/Chart/hooks/updateUtils.ts @@ -22,9 +22,9 @@ export function removeSeries( const updatedOptions = { ...currentOptions }; if (updatedOptions.series) { - updatedOptions.series = updatedOptions.series.filter( - series => series.name !== name, - ); + updatedOptions.series = [ + ...updatedOptions.series.filter(series => series.name !== name), + ]; } return updatedOptions; diff --git a/charts/core/src/Chart/hooks/useChart.ts b/charts/core/src/Chart/hooks/useChart.ts index 5417c60531..c62ab73e8f 100644 --- a/charts/core/src/Chart/hooks/useChart.ts +++ b/charts/core/src/Chart/hooks/useChart.ts @@ -74,7 +74,12 @@ export function useChart({ theme, onChartReady }: ChartHookProps) { const updateChartRef = useMemo( () => debounce((chartOptions: Partial) => { - chartInstanceRef.current?.setOption(chartOptions); + /** + * The second argument is `true` to merge the new options with the existing ones. + * This is needed to ensure that series get removed properly. + * See issue: https://github.com/apache/echarts/issues/6202 + * */ + chartInstanceRef.current?.setOption(chartOptions, true); }, 50), [], ); diff --git a/charts/core/src/Line/Line.tsx b/charts/core/src/Line/Line.tsx index 694fb94b0a..91e9d62702 100644 --- a/charts/core/src/Line/Line.tsx +++ b/charts/core/src/Line/Line.tsx @@ -6,7 +6,7 @@ import { defaultLineOptions } from './config'; import { LineProps } from './Line.types'; export function Line({ name, data }: LineProps) { - const { addChartSeries } = useChartContext(); + const { addChartSeries, removeChartSeries } = useChartContext(); useEffect(() => { addChartSeries({ @@ -14,6 +14,14 @@ export function Line({ name, data }: LineProps) { name, data, }); + + return () => { + /** + * Remove the series when the component unmounts to make sure the series + * is removed when a `Line` is hidden. + */ + removeChartSeries(name); + }; }, [name, data]); return null; From 9d82b66acfce431c66f97d1dc559ce375ce4f2c5 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 13:58:11 -0400 Subject: [PATCH 02/10] Cleanup Grid --- charts/core/src/Grid/Grid.tsx | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/charts/core/src/Grid/Grid.tsx b/charts/core/src/Grid/Grid.tsx index 01f80e605a..10c9bdb062 100644 --- a/charts/core/src/Grid/Grid.tsx +++ b/charts/core/src/Grid/Grid.tsx @@ -26,6 +26,24 @@ export function Grid({ horizontal = true, vertical = true }: GridProps) { updatedOptions.xAxis = getUpdatedLineOptions(!!vertical); updatedOptions.yAxis = getUpdatedLineOptions(!!horizontal); updateChartOptions(updatedOptions); + + return () => { + /** + * Hides the grid lines when the component is unmounted. + */ + updateChartOptions({ + xAxis: { + splitLine: { + show: false, + }, + }, + yAxis: { + splitLine: { + show: false, + }, + }, + }); + }; }, [horizontal, vertical, theme]); return null; From 9611398ed912ba1b95fbd5c633167aca0bb3429a Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 14:31:27 -0400 Subject: [PATCH 03/10] Fix axes and grid --- charts/core/src/XAxis/XAxis.tsx | 16 ++++++++++++++++ charts/core/src/YAxis/YAxis.tsx | 16 ++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/charts/core/src/XAxis/XAxis.tsx b/charts/core/src/XAxis/XAxis.tsx index ebca501ad3..0b7d20a264 100644 --- a/charts/core/src/XAxis/XAxis.tsx +++ b/charts/core/src/XAxis/XAxis.tsx @@ -97,6 +97,22 @@ export function XAxis({ type, label, unit }: XAxisProps) { useEffect(() => { updateChartOptions(getOptions({ type, label, unit, theme })); + + return () => { + /** + * Hides the axis when the component is unmounted. + */ + updateChartOptions({ + xAxis: { + axisLine: { + show: false, + }, + axisLabel: { + show: false, + }, + }, + }); + }; }, [type, label, unit, theme, updateChartOptions]); return null; diff --git a/charts/core/src/YAxis/YAxis.tsx b/charts/core/src/YAxis/YAxis.tsx index dea1e73319..40fceb7977 100644 --- a/charts/core/src/YAxis/YAxis.tsx +++ b/charts/core/src/YAxis/YAxis.tsx @@ -96,6 +96,22 @@ export function YAxis({ type, label, unit }: YAxisProps) { useEffect(() => { updateChartOptions(getOptions({ type, label, unit, theme })); + + return () => { + /** + * Hides the axis when the component is unmounted. + */ + updateChartOptions({ + yAxis: { + axisLine: { + show: false, + }, + axisLabel: { + show: false, + }, + }, + }); + }; }, [type, label, unit, theme, updateChartOptions]); return null; From 3a3f230f6008749c39569a54aaab1f118f4adedd Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 14:39:48 -0400 Subject: [PATCH 04/10] Changeset --- .changeset/nice-radios-move.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/nice-radios-move.md diff --git a/.changeset/nice-radios-move.md b/.changeset/nice-radios-move.md new file mode 100644 index 0000000000..6fe6f55dd0 --- /dev/null +++ b/.changeset/nice-radios-move.md @@ -0,0 +1,5 @@ +--- +'@lg-charts/core': patch +--- + +Fixes hiding chart elements when components are removed. From d6bc99e6879ceafde212db4a16097b11969cb59a Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 15:28:23 -0400 Subject: [PATCH 05/10] Move unset options into var --- charts/core/src/Grid/Grid.tsx | 18 ++++++++---------- charts/core/src/XAxis/XAxis.tsx | 18 ++++++++++-------- charts/core/src/YAxis/YAxis.tsx | 18 ++++++++++-------- 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/charts/core/src/Grid/Grid.tsx b/charts/core/src/Grid/Grid.tsx index 10c9bdb062..4dfbcd8de1 100644 --- a/charts/core/src/Grid/Grid.tsx +++ b/charts/core/src/Grid/Grid.tsx @@ -8,6 +8,12 @@ import { useChartContext } from '../ChartContext'; import { GridProps } from './Grid.types'; +const unsetGridOptions = { + splitLine: { + show: false, + }, +}; + export function Grid({ horizontal = true, vertical = true }: GridProps) { const { updateChartOptions } = useChartContext(); const { theme } = useDarkMode(); @@ -32,16 +38,8 @@ export function Grid({ horizontal = true, vertical = true }: GridProps) { * Hides the grid lines when the component is unmounted. */ updateChartOptions({ - xAxis: { - splitLine: { - show: false, - }, - }, - yAxis: { - splitLine: { - show: false, - }, - }, + xAxis: unsetGridOptions, + yAxis: unsetGridOptions, }); }; }, [horizontal, vertical, theme]); diff --git a/charts/core/src/XAxis/XAxis.tsx b/charts/core/src/XAxis/XAxis.tsx index 0b7d20a264..1ff98d044e 100644 --- a/charts/core/src/XAxis/XAxis.tsx +++ b/charts/core/src/XAxis/XAxis.tsx @@ -77,6 +77,15 @@ const getOptions = ({ return options; }; +const unsetAxisOptions = { + axisLine: { + show: false, + }, + axisLabel: { + show: false, + }, +}; + /** * React component that can render an x-axis on a parent chart. * @@ -103,14 +112,7 @@ export function XAxis({ type, label, unit }: XAxisProps) { * Hides the axis when the component is unmounted. */ updateChartOptions({ - xAxis: { - axisLine: { - show: false, - }, - axisLabel: { - show: false, - }, - }, + xAxis: unsetAxisOptions, }); }; }, [type, label, unit, theme, updateChartOptions]); diff --git a/charts/core/src/YAxis/YAxis.tsx b/charts/core/src/YAxis/YAxis.tsx index 40fceb7977..a4e7d629e4 100644 --- a/charts/core/src/YAxis/YAxis.tsx +++ b/charts/core/src/YAxis/YAxis.tsx @@ -76,6 +76,15 @@ const getOptions = ({ return options; }; +const unsetAxisOptions = { + axisLine: { + show: false, + }, + axisLabel: { + show: false, + }, +}; + /** * React component that can render an y-axis on a parent chart. * @@ -102,14 +111,7 @@ export function YAxis({ type, label, unit }: YAxisProps) { * Hides the axis when the component is unmounted. */ updateChartOptions({ - yAxis: { - axisLine: { - show: false, - }, - axisLabel: { - show: false, - }, - }, + yAxis: unsetAxisOptions, }); }; }, [type, label, unit, theme, updateChartOptions]); From 06ba916f760db8e8d17ab1c0691d09daaf06f5db Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 19:06:05 -0400 Subject: [PATCH 06/10] Prevent series duplication --- charts/core/README.md | 8 ++++---- charts/core/src/Chart/hooks/updateUtils.ts | 4 +++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/charts/core/README.md b/charts/core/README.md index c7940d1925..b518e0d4ec 100644 --- a/charts/core/README.md +++ b/charts/core/README.md @@ -70,10 +70,10 @@ Component that takes in data points and renders a single line on the chart. #### Props -| Name | Description | Type | Default | -| ------ | ---------------------------------------------------------------------- | ----------------------------------------------------------- | ------- | -| `name` | Name used to identify the series. | string | | -| `data` | Data array of tuples that represent x and y coordinates in the series. | Array<[string \| number \| Date, string \| number \| Date]> | | +| Name | Description | Type | Default | +| ------ | ------------------------------------------------------------------------------------------------------------------------ | ----------------------------------------------------------- | ------- | +| `name` | Unique name used to identify the series. **Important note:** If two lines have the same name, only one will be rendered. | string | | +| `data` | Data array of tuples that represent x and y coordinates in the series. | Array<[string \| number \| Date, string \| number \| Date]> | | ### `Grid` diff --git a/charts/core/src/Chart/hooks/updateUtils.ts b/charts/core/src/Chart/hooks/updateUtils.ts index fa4aa98b3b..a41dd13d78 100644 --- a/charts/core/src/Chart/hooks/updateUtils.ts +++ b/charts/core/src/Chart/hooks/updateUtils.ts @@ -9,7 +9,9 @@ export function addSeries( if (!updatedOptions.series) { updatedOptions.series = [data]; } else { - updatedOptions.series.push(data); + if (!updatedOptions.series.some(series => series.name === data.name)) { + updatedOptions.series.push(data); + } } return updatedOptions; From 8c2dc53786f96faf82fb29ebf28c2dc7d866fa72 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 19:07:12 -0400 Subject: [PATCH 07/10] Add test --- charts/core/src/Chart/hooks/updateUtils.spec.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/charts/core/src/Chart/hooks/updateUtils.spec.ts b/charts/core/src/Chart/hooks/updateUtils.spec.ts index 710bd4c893..35a3a9f8f4 100644 --- a/charts/core/src/Chart/hooks/updateUtils.spec.ts +++ b/charts/core/src/Chart/hooks/updateUtils.spec.ts @@ -14,6 +14,17 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { expect(updatedOptions.series?.[1].name).toBe(newSeriesName); }); + test('should not a series to the chart options if a chart with the same name exists', () => { + const currentOptions: Partial = { + series: [{ name: 'series1' }], + }; + const newSeriesName = 'series1'; + const data = { name: newSeriesName }; + const updatedOptions = addSeries(currentOptions, data); + expect(updatedOptions.series).toHaveLength(1); + expect(updatedOptions.series?.[0].name).toBe(newSeriesName); + }); + test('should remove a series from the chart options', () => { const currentOptions: Partial = { series: [{ name: 'series1' }, { name: 'series2' }], From e08d2f2099a5a2498e6ac9b359ad530fdc14e495 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 19:12:02 -0400 Subject: [PATCH 08/10] Update patch description --- .changeset/nice-radios-move.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.changeset/nice-radios-move.md b/.changeset/nice-radios-move.md index 6fe6f55dd0..eeab95cbf6 100644 --- a/.changeset/nice-radios-move.md +++ b/.changeset/nice-radios-move.md @@ -2,4 +2,5 @@ '@lg-charts/core': patch --- -Fixes hiding chart elements when components are removed. +- Fixes hiding chart elements when components are removed. +- Fixes adding of duplicate series. From 436c01faf8c4d70f00a3dc1e63cb34f2a762ad24 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Wed, 30 Oct 2024 19:12:52 -0400 Subject: [PATCH 09/10] Fix test description --- charts/core/src/Chart/hooks/updateUtils.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/charts/core/src/Chart/hooks/updateUtils.spec.ts b/charts/core/src/Chart/hooks/updateUtils.spec.ts index 35a3a9f8f4..8f88e022af 100644 --- a/charts/core/src/Chart/hooks/updateUtils.spec.ts +++ b/charts/core/src/Chart/hooks/updateUtils.spec.ts @@ -14,7 +14,7 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { expect(updatedOptions.series?.[1].name).toBe(newSeriesName); }); - test('should not a series to the chart options if a chart with the same name exists', () => { + test('should not add a series if a chart with the same name exists', () => { const currentOptions: Partial = { series: [{ name: 'series1' }], }; From 90888aadd40d46e9b9e32f9cf0a0f7a97def5416 Mon Sep 17 00:00:00 2001 From: Terrence Keane Date: Fri, 1 Nov 2024 11:14:04 -0400 Subject: [PATCH 10/10] More descriptive test names --- charts/core/src/Chart/hooks/updateUtils.spec.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/charts/core/src/Chart/hooks/updateUtils.spec.ts b/charts/core/src/Chart/hooks/updateUtils.spec.ts index 8f88e022af..e87a86141e 100644 --- a/charts/core/src/Chart/hooks/updateUtils.spec.ts +++ b/charts/core/src/Chart/hooks/updateUtils.spec.ts @@ -3,7 +3,7 @@ import { ChartOptions } from '../Chart.types'; import { addSeries, removeSeries, updateOptions } from './updateUtils'; describe('@lg-charts/core/Chart/hooks/updateUtils', () => { - test('should add a series to the chart options', () => { + test('addSeries should add a series to the chart options', () => { const currentOptions: Partial = { series: [{ name: 'series1' }], }; @@ -14,7 +14,7 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { expect(updatedOptions.series?.[1].name).toBe(newSeriesName); }); - test('should not add a series if a chart with the same name exists', () => { + test('addSeries should not add a series if a chart with the same name exists', () => { const currentOptions: Partial = { series: [{ name: 'series1' }], }; @@ -25,7 +25,7 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { expect(updatedOptions.series?.[0].name).toBe(newSeriesName); }); - test('should remove a series from the chart options', () => { + test('removeSeries should remove a series from the chart options', () => { const currentOptions: Partial = { series: [{ name: 'series1' }, { name: 'series2' }], }; @@ -39,7 +39,7 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => { /** * Tests that option updates don't overwrite the entire chart options object. */ - test('should merge chart options non-destructively', () => { + test('updateOptions should merge chart options non-destructively', () => { const currentOptions: Partial = { xAxis: { show: true,