From aa5f1521fc05b70d5505a7b59cb0f21263e4a8eb Mon Sep 17 00:00:00 2001 From: Matt Vickers Date: Mon, 2 Dec 2024 15:58:51 -0600 Subject: [PATCH] Fix issue where legends would get formatted twice --- packages/polaris-viz/CHANGELOG.md | 6 +- .../components/LegendValues/LegendValues.tsx | 2 - .../LegendValueItem/LegendValueItem.tsx | 4 +- .../DonutChart/tests/DonutChart.test.tsx | 117 ++++++++++-------- .../components/HiddenLegendTooltip.tsx | 4 - .../tests/LegendsContainer.test.tsx | 11 +- .../SparkLineChart/tests/Chart.test.tsx | 2 +- .../tests/useGetLongestLabelFromData.test.tsx | 32 ++--- .../tests/useWatchColorVisionEvents.test.tsx | 26 ++-- .../tests/useLinearChartAnimations.test.tsx | 35 +++--- .../src/hooks/useLinearChartAnimations.tsx | 12 +- tests/setup/tests.ts | 21 ++++ 12 files changed, 153 insertions(+), 119 deletions(-) diff --git a/packages/polaris-viz/CHANGELOG.md b/packages/polaris-viz/CHANGELOG.md index 85a74e61b..d69996039 100644 --- a/packages/polaris-viz/CHANGELOG.md +++ b/packages/polaris-viz/CHANGELOG.md @@ -5,7 +5,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/) and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html). - +## Unreleased + +### Fixed + +- Fixed issue where `` would run the `seriesNameFormatter` for multiple times on a ``. ## [15.3.3] - 2024-12-02 diff --git a/packages/polaris-viz/src/components/DonutChart/components/LegendValues/LegendValues.tsx b/packages/polaris-viz/src/components/DonutChart/components/LegendValues/LegendValues.tsx index 81dd98bc3..531d3b2a4 100644 --- a/packages/polaris-viz/src/components/DonutChart/components/LegendValues/LegendValues.tsx +++ b/packages/polaris-viz/src/components/DonutChart/components/LegendValues/LegendValues.tsx @@ -142,7 +142,6 @@ export function LegendValues({ longestLegendValueWidth={longestLegendValueWidth} maxTrendIndicatorWidth={maxTrendIndicatorWidth} seriesColors={seriesColors} - seriesNameFormatter={seriesNameFormatter} onDimensionChange={(dimensions) => { if (legendItemDimensions.current) { legendItemDimensions.current[index] = dimensions; @@ -166,7 +165,6 @@ export function LegendValues({ label={renderHiddenLegendLabel(allData.length - displayedData.length)} lastVisibleIndex={allData.length - hiddenData.length} setActivatorWidth={() => null} - seriesNameFormatter={seriesNameFormatter} /> )} diff --git a/packages/polaris-viz/src/components/DonutChart/components/LegendValues/components/LegendValueItem/LegendValueItem.tsx b/packages/polaris-viz/src/components/DonutChart/components/LegendValues/components/LegendValueItem/LegendValueItem.tsx index 761f28c9d..102b68767 100644 --- a/packages/polaris-viz/src/components/DonutChart/components/LegendValues/components/LegendValueItem/LegendValueItem.tsx +++ b/packages/polaris-viz/src/components/DonutChart/components/LegendValues/components/LegendValueItem/LegendValueItem.tsx @@ -23,7 +23,6 @@ interface Props { labelFormatter: LabelFormatter; longestLegendValueWidth: number; seriesColors: Color[]; - seriesNameFormatter: LabelFormatter; maxTrendIndicatorWidth: number; onDimensionChange: (dimensions: Dimensions) => void; getColorVisionStyles: ColorVisionInteractionMethods['getColorVisionStyles']; @@ -38,7 +37,6 @@ export function LegendValueItem({ longestLegendValueWidth, trend, seriesColors, - seriesNameFormatter, maxTrendIndicatorWidth, onDimensionChange, getColorVisionStyles, @@ -79,7 +77,7 @@ export function LegendValueItem({ }} title={name} > - {seriesNameFormatter(name)} + {name} diff --git a/packages/polaris-viz/src/components/DonutChart/tests/DonutChart.test.tsx b/packages/polaris-viz/src/components/DonutChart/tests/DonutChart.test.tsx index 4426488fb..3a4cbac32 100644 --- a/packages/polaris-viz/src/components/DonutChart/tests/DonutChart.test.tsx +++ b/packages/polaris-viz/src/components/DonutChart/tests/DonutChart.test.tsx @@ -1,11 +1,16 @@ import {mount} from '@shopify/react-testing'; -import {ChartState, THIN_ARC_CORNER_THICKNESS} from '@shopify/polaris-viz-core'; +import { + ChartState, + THIN_ARC_CORNER_THICKNESS, + useChartContext, +} from '@shopify/polaris-viz-core'; -import {Chart as DonutChart} from '../Chart'; +import {DonutChart} from '../DonutChart'; import type {ChartProps} from '../Chart'; -import {Arc, InnerValue} from '../components'; +import {InnerValue} from '../components'; import {ComparisonMetric} from '../../ComparisonMetric'; import {LegendContainer} from '../../LegendContainer'; +import {Arc} from '../../Arc'; jest.mock('../components', () => ({ ...jest.requireActual('../components'), @@ -17,13 +22,14 @@ jest.mock('@shopify/polaris-viz-core/src/utilities', () => ({ changeColorOpacity: jest.fn(() => 'black'), })); +const mockUseChartContext = useChartContext as jest.Mock; + describe('', () => { describe('', () => { const mockProps: ChartProps = { showLegend: true, theme: `Light`, labelFormatter: (value) => `${value}`, - containerDimensions: {width: 500, height: 500}, data: [ { name: 'Shopify Payments', @@ -58,36 +64,31 @@ describe('', () => { it('renders an SVG element', () => { const chart = mount(); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponent('svg'); - }); - }); + + expect(chart).toContainReactComponent('svg'); }); it('renders total value', () => { const chart = mount(); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponent(InnerValue, { - totalValue: valueSum, - }); - }); + expect(chart).toContainReactComponent(InnerValue, { + totalValue: valueSum, }); }); it('renders a thinner when container height is small', () => { - const chart = mount( - , - ); - - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponent(Arc, { - thickness: THIN_ARC_CORNER_THICKNESS, - }); - }); + mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), + containerBounds: { + width: 600, + height: 100, + }, + }); + + const chart = mount(); + + expect(chart).toContainReactComponent(Arc, { + thickness: THIN_ARC_CORNER_THICKNESS, }); }); @@ -97,24 +98,16 @@ describe('', () => { , ); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).not.toContainReactComponent(ComparisonMetric); - }); - }); + expect(chart).not.toContainReactComponent(ComparisonMetric); }); it('renders if comparisonMetric is provided', () => { const chart = mount(); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponent( - ComparisonMetric, - mockProps.comparisonMetric, - ); - }); - }); + expect(chart).toContainReactComponent( + ComparisonMetric, + mockProps.comparisonMetric, + ); }); }); @@ -141,7 +134,7 @@ describe('', () => { />, ); - expect(chart).not.toContainReactComponent(LegendContainer, { + expect(chart).toContainReactComponent(LegendContainer, { renderLegendContent, }); }); @@ -150,11 +143,8 @@ describe('', () => { describe('empty state', () => { it('renders single when true', () => { const chart = mount(); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponentTimes(Arc, 1); - }); - }); + + expect(chart).toContainReactComponentTimes(Arc, 1); }); it('renders the empty state if all data values are 0', () => { @@ -174,20 +164,39 @@ describe('', () => { />, ); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponentTimes(Arc, 1); - }); - }); + expect(chart).toContainReactComponentTimes(Arc, 1); }); it('renders multiple when false', () => { const chart = mount(); - chart.act(() => { - requestAnimationFrame(() => { - expect(chart).toContainReactComponentTimes(Arc, 4); - }); - }); + + expect(chart).toContainReactComponentTimes(Arc, 4); + }); + }); + + describe('seriesNameFormatter', () => { + it('formats series name', () => { + const chart = mount( + `series: ${name}`} + />, + ); + + expect(chart).toContainReactText('series: Shopify Payments'); + }); + + it('does not format the series name twice', () => { + const chart = mount( + `series: ${name}`} + />, + ); + + expect(chart).not.toContainReactText( + 'series: series: Shopify Payments', + ); }); }); }); diff --git a/packages/polaris-viz/src/components/LegendContainer/components/HiddenLegendTooltip.tsx b/packages/polaris-viz/src/components/LegendContainer/components/HiddenLegendTooltip.tsx index 0f72a9c2c..ae934eaa0 100644 --- a/packages/polaris-viz/src/components/LegendContainer/components/HiddenLegendTooltip.tsx +++ b/packages/polaris-viz/src/components/LegendContainer/components/HiddenLegendTooltip.tsx @@ -13,7 +13,6 @@ import { useChartContext, useTheme, } from '@shopify/polaris-viz-core'; -import type {LabelFormatter} from '@shopify/polaris-viz-core'; import {getFontSize} from '../../../utilities/getFontSize'; import type {LegendData} from '../../../types'; @@ -34,7 +33,6 @@ interface Props { setActivatorWidth: (width: number) => void; theme?: string; lastVisibleIndex?: number; - seriesNameFormatter?: LabelFormatter; } export const LEGEND_TOOLTIP_ID = 'legend-toolip'; @@ -48,7 +46,6 @@ export function HiddenLegendTooltip({ label, lastVisibleIndex = 0, setActivatorWidth, - seriesNameFormatter, }: Props) { const selectedTheme = useTheme(); const {isFirefox} = useBrowserCheck(); @@ -173,7 +170,6 @@ export function HiddenLegendTooltip({ theme={theme} indexOffset={lastVisibleIndex} backgroundColor="transparent" - seriesNameFormatter={seriesNameFormatter} /> , container, diff --git a/packages/polaris-viz/src/components/LegendContainer/tests/LegendsContainer.test.tsx b/packages/polaris-viz/src/components/LegendContainer/tests/LegendsContainer.test.tsx index 9dfd1f671..a34c9b8ee 100644 --- a/packages/polaris-viz/src/components/LegendContainer/tests/LegendsContainer.test.tsx +++ b/packages/polaris-viz/src/components/LegendContainer/tests/LegendsContainer.test.tsx @@ -3,10 +3,10 @@ import {useChartContext} from '@shopify/polaris-viz-core'; import type {LegendContainerProps} from '../LegendContainer'; import {LegendContainer} from '../LegendContainer'; -import {Legend, LegendItem} from '../../Legend'; +import {Legend} from '../../Legend'; import {HiddenLegendTooltip} from '../components/HiddenLegendTooltip'; -const WIDTH_WITH_OVERFLOW = 0; +const WIDTH_WITH_OVERFLOW = 10; const WIDTH_WITHOUT_OVERFLOW = 100; const mockProps: LegendContainerProps = { @@ -30,12 +30,6 @@ jest.mock('../../../hooks/useResizeObserver', () => { }; }); -jest.mock('@shopify/polaris-viz-core/src/hooks/useChartContext', () => ({ - useChartContext: jest.fn(() => ({ - containerBounds: {height: 300, width: 100}, - })), -})); - const mockUseChartContext = useChartContext as jest.Mock; describe('', () => { @@ -166,6 +160,7 @@ describe('', () => { it('renders a custom label if provided', () => { mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), containerBounds: {height: 300, width: WIDTH_WITH_OVERFLOW}, }); diff --git a/packages/polaris-viz/src/components/SparkLineChart/tests/Chart.test.tsx b/packages/polaris-viz/src/components/SparkLineChart/tests/Chart.test.tsx index 219440801..34b95632e 100644 --- a/packages/polaris-viz/src/components/SparkLineChart/tests/Chart.test.tsx +++ b/packages/polaris-viz/src/components/SparkLineChart/tests/Chart.test.tsx @@ -26,7 +26,7 @@ describe('', () => { const offsetLeft = 100; const offsetRight = 50; const margin = 2; - const mockWidth = 0; + const mockWidth = 600; mount( { - return { - ...jest.requireActual('react'), - useContext: () => ({ - characterWidths: { - // eslint-disable-next-line id-length - a: 1, - // eslint-disable-next-line id-length - b: 1, - 1: 1, - 0: 1, - }, - }), - }; -}); +// jest.mock('react', () => { +// return { +// ...jest.requireActual('react'), +// useContext: () => ({ +// characterWidths: { +// // eslint-disable-next-line id-length +// a: 1, +// // eslint-disable-next-line id-length +// b: 1, +// 1: 1, +// 0: 1, +// }, +// }), +// }; +// }); function parseData(result: Root) { return JSON.parse(result.domNode?.dataset.data ?? ''); @@ -54,7 +54,7 @@ describe('useGetLongestLabelFromData()', () => { const data = parseData(result); - expect(data).toStrictEqual({keyWidth: 1, valueWidth: 4}); + expect(data).toStrictEqual({keyWidth: 6.63, valueWidth: 22.32}); }); it('does not throw error when data is missing', () => { diff --git a/packages/polaris-viz/src/hooks/ColorVisionA11y/tests/useWatchColorVisionEvents.test.tsx b/packages/polaris-viz/src/hooks/ColorVisionA11y/tests/useWatchColorVisionEvents.test.tsx index 35ff8d28c..49eee8989 100644 --- a/packages/polaris-viz/src/hooks/ColorVisionA11y/tests/useWatchColorVisionEvents.test.tsx +++ b/packages/polaris-viz/src/hooks/ColorVisionA11y/tests/useWatchColorVisionEvents.test.tsx @@ -1,5 +1,9 @@ +import {mount} from '@shopify/react-testing'; +import {useChartContext} from '@shopify/polaris-viz-core'; + import {useWatchColorVisionEvents} from '../useWatchColorVisionEvents'; -import {mountWithChartContext} from '../../../test-utilities/mountWithChartContext'; + +const mockUseChartContext = useChartContext as jest.Mock; describe('useWatchColorVisionEvents', () => { let events: {[key: string]: any} = {}; @@ -35,12 +39,17 @@ describe('useWatchColorVisionEvents', () => { return null; } - mountWithChartContext(, {id: null}); + mount(); expect(Object.keys(events)).toHaveLength(0); }); it('attaches events to the window', () => { + mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), + id: '123', + }); + function TestComponent() { useWatchColorVisionEvents({ type: 'someType', @@ -50,9 +59,7 @@ describe('useWatchColorVisionEvents', () => { return null; } - mountWithChartContext(, { - id: '123', - }); + mount(); expect(Object.keys(events)).toHaveLength(1); expect(Object.keys(events)[0]).toStrictEqual( @@ -61,6 +68,11 @@ describe('useWatchColorVisionEvents', () => { }); it('responds to event', () => { + mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), + id: '123', + }); + const spy = jest.fn(); function TestComponent() { @@ -72,9 +84,7 @@ describe('useWatchColorVisionEvents', () => { return null; } - mountWithChartContext(, { - id: '123', - }); + mount(); events['123:color-vision-event:someType'](); diff --git a/packages/polaris-viz/src/hooks/tests/useLinearChartAnimations.test.tsx b/packages/polaris-viz/src/hooks/tests/useLinearChartAnimations.test.tsx index 712c904aa..43f122efb 100644 --- a/packages/polaris-viz/src/hooks/tests/useLinearChartAnimations.test.tsx +++ b/packages/polaris-viz/src/hooks/tests/useLinearChartAnimations.test.tsx @@ -1,12 +1,10 @@ -/* eslint-disable react/jsx-no-constructed-context-values */ import {mount} from '@shopify/react-testing'; import {line} from 'd3-shape'; import type {LineChartDataSeriesWithDefaults} from '@shopify/polaris-viz-core'; -import {ChartContext} from '@shopify/polaris-viz-core'; +import {useChartContext} from '@shopify/polaris-viz-core'; +import type {Props} from '../useLinearChartAnimations'; import {useLinearChartAnimations} from '../useLinearChartAnimations'; -import characterWidths from '../../data/character-widths.json'; -import characterWidthOffsets from '../../data/character-width-offsets.json'; jest.mock('../../utilities/getPathLength', () => { return { @@ -26,6 +24,8 @@ const lineGeneratorMock = jest.fn( .y(({value}) => value), ) as any; +const mockUseChartContext = useChartContext as jest.Mock; + const data: LineChartDataSeriesWithDefaults[] = [ { name: 'Primary', @@ -43,14 +43,20 @@ const data: LineChartDataSeriesWithDefaults[] = [ }, ]; -const mockProps = { +const mockProps: Props = { activeIndex: 0, lineGenerator: lineGeneratorMock, data, - isAnimated: true, }; describe('useLinearChartAnimations', () => { + beforeEach(() => { + mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), + shouldAnimate: true, + }); + }); + afterEach(() => { jest.clearAllMocks(); }); @@ -111,17 +117,12 @@ describe('useLinearChartAnimations', () => { return null; } - mount( - - - , - ); + mockUseChartContext.mockReturnValue({ + ...mockUseChartContext(), + shouldAnimate: false, + }); + + mount(); expect(lineGeneratorMock).not.toHaveBeenCalled(); }); diff --git a/packages/polaris-viz/src/hooks/useLinearChartAnimations.tsx b/packages/polaris-viz/src/hooks/useLinearChartAnimations.tsx index 966cdd382..09b07637d 100644 --- a/packages/polaris-viz/src/hooks/useLinearChartAnimations.tsx +++ b/packages/polaris-viz/src/hooks/useLinearChartAnimations.tsx @@ -13,15 +13,17 @@ export const SPRING_CONFIG = { tension: 190, }; +export interface Props { + activeIndex: number | null; + lineGenerator: Line; + data: DataSeries[]; +} + export function useLinearChartAnimations({ activeIndex, lineGenerator, data, -}: { - activeIndex: number | null; - lineGenerator: Line; - data: DataSeries[]; -}) { +}: Props) { const {shouldAnimate} = useChartContext(); const currentIndex = activeIndex == null ? 0 : activeIndex; diff --git a/tests/setup/tests.ts b/tests/setup/tests.ts index fd6b0df4a..0f32047c2 100644 --- a/tests/setup/tests.ts +++ b/tests/setup/tests.ts @@ -4,6 +4,9 @@ import {cloneElement} from 'react'; // eslint-disable-next-line import/no-extraneous-dependencies import {Globals} from '@react-spring/web'; +import mockCharacterWidths from '../../packages/polaris-viz/src/data/character-widths.json'; +import mockCharacterWidthOffsets from '../../packages/polaris-viz/src/data/character-width-offsets.json'; + const mockCloneElement = (element: React.ReactElement, props) => cloneElement(element, props); @@ -19,6 +22,24 @@ jest.mock('../../packages/polaris-viz/src/components/ChartContainer', () => { }; }); +jest.mock('../../packages/polaris-viz-core/src/hooks/useChartContext', () => { + return { + useChartContext: jest.fn().mockReturnValue({ + shouldAnimate: false, + id: null, + characterWidths: mockCharacterWidths, + characterWidthOffsets: mockCharacterWidthOffsets, + theme: 'Light', + isTouchDevice: false, + isPerformanceImpacted: false, + containerBounds: { + width: 600, + height: 400, + }, + }), + }; +}); + jest.mock( '../../packages/polaris-viz-core/src/styles/shared/_variables.scss', () => {