From 8d591a6a0026829246582c0197adbe49fde5cf5d Mon Sep 17 00:00:00 2001 From: annacmc Date: Thu, 23 Jan 2025 16:35:02 +1100 Subject: [PATCH 1/9] add bar chart tests --- .../bar-chart/test/bar-chart.test.tsx | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx diff --git a/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx b/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx new file mode 100644 index 0000000000000..10c75280594c2 --- /dev/null +++ b/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx @@ -0,0 +1,161 @@ +/** + * @jest-environment jsdom + */ + +import { render, screen } from '@testing-library/react'; +import { ThemeProvider } from '../../../providers/theme'; +import BarChart from '../bar-chart'; + +describe( 'BarChart', () => { + const defaultProps = { + width: 500, + height: 300, + data: [ + { + label: 'Series A', + data: [ + { date: new Date( '2024-01-01' ), value: 10, label: 'Jan 1' }, + { date: new Date( '2024-01-02' ), value: 20, label: 'Jan 2' }, + ], + options: {}, + }, + ], + }; + + const renderWithTheme = ( props = {} ) => { + return render( + + + + ); + }; + + describe( 'Data Validation', () => { + test( 'handles empty data array', () => { + renderWithTheme( { data: [] } ); + expect( screen.getByText( /no data available/i ) ).toBeInTheDocument(); + } ); + + test( 'handles single data point', () => { + renderWithTheme( { + data: [ + { + label: 'Series A', + data: [ { date: new Date( '2024-01-01' ), value: 10, label: 'Jan 1' } ], + options: {}, + }, + ], + } ); + expect( screen.getByRole( 'img', { name: /bar chart/i } ) ).toBeInTheDocument(); + } ); + + test( 'handles negative values', () => { + renderWithTheme( { + data: [ + { + label: 'Series A', + data: [ + { date: new Date( '2024-01-01' ), value: -10, label: 'Jan 1' }, + { date: new Date( '2024-01-02' ), value: -20, label: 'Jan 2' }, + ], + options: {}, + }, + ], + } ); + expect( screen.getByRole( 'img', { name: /bar chart/i } ) ).toBeInTheDocument(); + } ); + + test( 'handles null or undefined values', () => { + renderWithTheme( { + data: [ + { + label: 'Series A', + data: [ + { date: new Date( '2024-01-01' ), value: null as number | null, label: 'Jan 1' }, + { + date: new Date( '2024-01-02' ), + value: undefined as number | undefined, + label: 'Jan 2', + }, + ], + options: {}, + }, + ], + } ); + expect( screen.getByText( /invalid data/i ) ).toBeInTheDocument(); + } ); + + test( 'handles invalid date values', () => { + renderWithTheme( { + data: [ + { + label: 'Series A', + data: [ + { date: new Date( 'invalid' ), value: 10, label: 'Jan 1' }, + { date: new Date( '2024-01-02' ), value: 20, label: 'Jan 2' }, + ], + options: {}, + }, + ], + } ); + expect( screen.getByText( /invalid data/i ) ).toBeInTheDocument(); + } ); + } ); + + describe( 'Legend', () => { + test( 'shows legend when showLegend is true', () => { + renderWithTheme( { + showLegend: true, + data: [ + { + label: 'Series A', + data: [ { date: new Date( '2024-01-01' ), value: 10, label: 'Jan 1' } ], + options: {}, + }, + { + label: 'Series B', + data: [ { date: new Date( '2024-01-01' ), value: 20, label: 'Jan 1' } ], + options: {}, + }, + ], + } ); + expect( screen.getByText( 'Series A' ) ).toBeInTheDocument(); + expect( screen.getByText( 'Series B' ) ).toBeInTheDocument(); + } ); + + test( 'hides legend when showLegend is false', () => { + renderWithTheme( { + showLegend: false, + data: [ + { + label: 'Series A', + data: [ { date: new Date( '2024-01-01' ), value: 10, label: 'Jan 1' } ], + options: {}, + }, + ], + } ); + expect( screen.queryByText( 'Series A' ) ).not.toBeInTheDocument(); + } ); + } ); + + describe( 'Grid Visibility', () => { + test( 'renders with different grid visibility options', () => { + const { rerender } = renderWithTheme( { gridVisibility: 'x' } ); + expect( screen.getByRole( 'img', { name: /bar chart/i } ) ).toBeInTheDocument(); + + rerender( + + + + ); + expect( screen.getByRole( 'img', { name: /bar chart/i } ) ).toBeInTheDocument(); + + rerender( + + + + ); + expect( screen.getByRole( 'img', { name: /bar chart/i } ) ).toBeInTheDocument(); + } ); + } ); +} ); From 70129d51d58fdc2a85ef7fc6e2728959159e72a6 Mon Sep 17 00:00:00 2001 From: annacmc Date: Thu, 23 Jan 2025 17:00:04 +1100 Subject: [PATCH 2/9] improve error handling + data validation --- .../src/components/bar-chart/bar-chart.tsx | 62 +++++++++++++------ .../bar-chart/test/bar-chart.test.tsx | 16 +++++ 2 files changed, 59 insertions(+), 19 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index c68bbed0b27a4..d3ce27f994907 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -23,21 +23,15 @@ type BarChartTooltipData = { interface BarChartProps extends BaseChartProps< SeriesData[] > {} -const BarChart: FC< BarChartProps > = ( { - data, - margin = { top: 20, right: 20, bottom: 40, left: 40 }, - withTooltips = false, - showLegend = false, - legendOrientation = 'horizontal', - className, - gridVisibility = 'x', - width, - height = 400, -} ) => { +const BarChart: FC< BarChartProps > = props => { const theme = useChartTheme(); const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< BarChartTooltipData >(); + const handleMouseLeave = useCallback( () => { + hideTooltip(); + }, [ hideTooltip ] ); + const handleMouseMove = useCallback( ( event: MouseEvent< SVGRectElement >, @@ -48,7 +42,6 @@ const BarChart: FC< BarChartProps > = ( { ) => { const coords = localPoint( event ); if ( ! coords ) return; - showTooltip( { tooltipData: { value, xLabel, yLabel, seriesIndex }, tooltipLeft: coords.x, @@ -58,12 +51,37 @@ const BarChart: FC< BarChartProps > = ( { [ showTooltip ] ); - const handleMouseLeave = useCallback( () => { - hideTooltip(); - }, [ hideTooltip ] ); - + const { + data, + margin = { top: 20, right: 20, bottom: 40, left: 40 }, + withTooltips = false, + showLegend = false, + legendOrientation = 'horizontal', + className, + gridVisibility = 'x', + width, + height = 400, + } = props; + + // Check for empty data if ( ! data?.length ) { - return
Empty...
; + return
No data available
; + } + + // Add date validation to hasInvalidData check + const hasInvalidData = data.some( series => + series.data.some( + d => + d.value === null || + d.value === undefined || + isNaN( d.value ) || + ! d.label || + ( d.date && isNaN( d.date.getTime() ) ) // Add date validation + ) + ); + + if ( hasInvalidData ) { + return
Invalid data
; } const margins = margin; @@ -102,7 +120,13 @@ const BarChart: FC< BarChartProps > = ( { } ) ); return ( -
+
= ( { ) }
diff --git a/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx b/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx index 10c75280594c2..3bdd93c405a67 100644 --- a/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/test/bar-chart.test.tsx @@ -100,6 +100,22 @@ describe( 'BarChart', () => { } ); expect( screen.getByText( /invalid data/i ) ).toBeInTheDocument(); } ); + + test( 'handles invalid label values', () => { + renderWithTheme( { + data: [ + { + label: 'Series A', + data: [ + { label: '', value: 10 }, // Empty label + { label: 'Label 2', value: 20 }, + ], + options: {}, + }, + ], + } ); + expect( screen.getByText( /invalid data/i ) ).toBeInTheDocument(); + } ); } ); describe( 'Legend', () => { From 5702338cece06ba56ba2a4446190eb5bd87f0d9f Mon Sep 17 00:00:00 2001 From: annacmc Date: Fri, 24 Jan 2025 15:00:07 +1100 Subject: [PATCH 3/9] add error state story --- .../bar-chart/stories/index.stories.tsx | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/projects/js-packages/charts/src/components/bar-chart/stories/index.stories.tsx b/projects/js-packages/charts/src/components/bar-chart/stories/index.stories.tsx index f5beaf7fe8436..6fb4a3f98947e 100644 --- a/projects/js-packages/charts/src/components/bar-chart/stories/index.stories.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/stories/index.stories.tsx @@ -106,3 +106,43 @@ export const FixedDimensions: Story = { }, }, }; + +export const ErrorStates: StoryObj< typeof BarChart > = { + render: () => ( +
+
+

Empty Data

+
+ +
+
+ +
+

Invalid Data

+
+ +
+
+
+ ), +}; + +ErrorStates.parameters = { + docs: { + description: { + story: + 'Examples of how the bar chart handles various error states including empty data and invalid data.', + }, + }, +}; From 68131f42dd593f632583065a5c1da10149ae2065 Mon Sep 17 00:00:00 2001 From: annacmc Date: Fri, 24 Jan 2025 15:00:53 +1100 Subject: [PATCH 4/9] changelog --- .../charts/changelog/add-chart-library-test-bar-chart | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 projects/js-packages/charts/changelog/add-chart-library-test-bar-chart diff --git a/projects/js-packages/charts/changelog/add-chart-library-test-bar-chart b/projects/js-packages/charts/changelog/add-chart-library-test-bar-chart new file mode 100644 index 0000000000000..9cf7990dc3e88 --- /dev/null +++ b/projects/js-packages/charts/changelog/add-chart-library-test-bar-chart @@ -0,0 +1,4 @@ +Significance: minor +Type: added + +Charts: adds tests and fixes to bar chart component From 07bd1c8849794013580c6d157abdffa0d831c358 Mon Sep 17 00:00:00 2001 From: annacmc Date: Tue, 28 Jan 2025 10:30:26 +1100 Subject: [PATCH 5/9] remove extra code not needed --- .../js-packages/charts/src/components/bar-chart/bar-chart.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index d3ce27f994907..0b132224f6d09 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -123,7 +123,6 @@ const BarChart: FC< BarChartProps > = props => {
From 3864d093b13871035c08e27a239a6623fe10c3a5 Mon Sep 17 00:00:00 2001 From: annacmc Date: Tue, 28 Jan 2025 11:15:05 +1100 Subject: [PATCH 6/9] Remove useCallback since it's not providing any benefit --- .../js-packages/charts/src/components/bar-chart/bar-chart.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index 0b132224f6d09..8c65d8fe4f24c 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -28,9 +28,9 @@ const BarChart: FC< BarChartProps > = props => { const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< BarChartTooltipData >(); - const handleMouseLeave = useCallback( () => { + const handleMouseLeave = () => { hideTooltip(); - }, [ hideTooltip ] ); + }; const handleMouseMove = useCallback( ( From 992f10f806a47bdbb0c6af0cab4704f3e6d38c6f Mon Sep 17 00:00:00 2001 From: annacmc Date: Tue, 28 Jan 2025 11:31:52 +1100 Subject: [PATCH 7/9] destructure BarChart props in function parameters --- .../src/components/bar-chart/bar-chart.tsx | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index 8c65d8fe4f24c..2a8716d0e6cc3 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -23,7 +23,17 @@ type BarChartTooltipData = { interface BarChartProps extends BaseChartProps< SeriesData[] > {} -const BarChart: FC< BarChartProps > = props => { +const BarChart: FC< BarChartProps > = ( { + data, + margin = { top: 20, right: 20, bottom: 40, left: 40 }, + withTooltips = false, + showLegend = false, + legendOrientation = 'horizontal', + className, + gridVisibility = 'x', + width, + height = 400, +} ) => { const theme = useChartTheme(); const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< BarChartTooltipData >(); @@ -51,18 +61,6 @@ const BarChart: FC< BarChartProps > = props => { [ showTooltip ] ); - const { - data, - margin = { top: 20, right: 20, bottom: 40, left: 40 }, - withTooltips = false, - showLegend = false, - legendOrientation = 'horizontal', - className, - gridVisibility = 'x', - width, - height = 400, - } = props; - // Check for empty data if ( ! data?.length ) { return
No data available
; From 90dd2be9f803f867778ee110b83514aa27f238d9 Mon Sep 17 00:00:00 2001 From: annacmc Date: Tue, 28 Jan 2025 15:22:50 +1100 Subject: [PATCH 8/9] add back in string class for debugging and external changes --- .../js-packages/charts/src/components/bar-chart/bar-chart.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index 2a8716d0e6cc3..e89306ec386a6 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -119,7 +119,7 @@ const BarChart: FC< BarChartProps > = ( { return (
Date: Thu, 30 Jan 2025 10:14:44 +1300 Subject: [PATCH 9/9] remove unnecessary wrapper of a function --- .../charts/src/components/bar-chart/bar-chart.tsx | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx index e89306ec386a6..794894a92a30c 100644 --- a/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx +++ b/projects/js-packages/charts/src/components/bar-chart/bar-chart.tsx @@ -38,10 +38,6 @@ const BarChart: FC< BarChartProps > = ( { const { tooltipOpen, tooltipLeft, tooltipTop, tooltipData, hideTooltip, showTooltip } = useTooltip< BarChartTooltipData >(); - const handleMouseLeave = () => { - hideTooltip(); - }; - const handleMouseMove = useCallback( ( event: MouseEvent< SVGRectElement >, @@ -154,7 +150,7 @@ const BarChart: FC< BarChartProps > = ( { height={ yMax - ( yScale( d.value ) ?? 0 ) } fill={ theme.colors[ seriesIndex % theme.colors.length ] } onMouseMove={ withTooltips ? handleBarMouseMove : undefined } - onMouseLeave={ withTooltips ? handleMouseLeave : undefined } + onMouseLeave={ withTooltips ? hideTooltip : undefined } /> ); } ) }