Fix issue where legends would get formatted twice

Shopify · Dec 3, 2024 · aa5f152 · aa5f152
1 parent 4adc8b1
commit aa5f152
Show file tree

Hide file tree

Showing 12 changed files with 153 additions and 119 deletions.
diff --git 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 -->
+## Unreleased
+
+### Fixed
+
+- Fixed issue where `<DonutChart />` would run the `seriesNameFormatter` for multiple times on a `<Legend />`.
 
 ## [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
@@ -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}
         />
       )}
     </React.Fragment>

diff --git a/...ponents/DonutChart/components/LegendValues/components/LegendValueItem/LegendValueItem.tsx b/...ponents/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}
       >
-        <span>{seriesNameFormatter(name)}</span>
+        <span>{name}</span>
       </td>
 
       <td className={styles.alignRight} width={longestLegendValueWidth}>

diff --git 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('<DonutChart />', () => {
   describe('<Chart/>', () => {
     const mockProps: ChartProps = {
       showLegend: true,
       theme: `Light`,
       labelFormatter: (value) => `${value}`,
-      containerDimensions: {width: 500, height: 500},
       data: [
         {
           name: 'Shopify Payments',
@@ -58,36 +64,31 @@ describe('<DonutChart />', () => {
 
     it('renders an SVG element', () => {
       const chart = mount(<DonutChart {...mockProps} />);
-      chart.act(() => {
-        requestAnimationFrame(() => {
-          expect(chart).toContainReactComponent('svg');
-        });
-      });
+
+      expect(chart).toContainReactComponent('svg');
     });
 
     it('renders total value', () => {
       const chart = mount(<DonutChart {...mockProps} />);
 
-      chart.act(() => {
-        requestAnimationFrame(() => {
-          expect(chart).toContainReactComponent(InnerValue, {
-            totalValue: valueSum,
-          });
-        });
+      expect(chart).toContainReactComponent(InnerValue, {
+        totalValue: valueSum,
       });
     });
 
     it('renders a thinner <Arc /> when container height is small', () => {
-      const chart = mount(
-        <DonutChart {...mockProps} dimensions={{width: 500, height: 150}} />,
-      );
-
-      chart.act(() => {
-        requestAnimationFrame(() => {
-          expect(chart).toContainReactComponent(Arc, {
-            thickness: THIN_ARC_CORNER_THICKNESS,
-          });
-        });
+      mockUseChartContext.mockReturnValue({
+        ...mockUseChartContext(),
+        containerBounds: {
+          width: 600,
+          height: 100,
+        },
+      });
+
+      const chart = mount(<DonutChart {...mockProps} />);
+
+      expect(chart).toContainReactComponent(Arc, {
+        thickness: THIN_ARC_CORNER_THICKNESS,
       });
     });
 
@@ -97,24 +98,16 @@ describe('<DonutChart />', () => {
           <DonutChart {...mockProps} comparisonMetric={undefined} />,
         );
 
-        chart.act(() => {
-          requestAnimationFrame(() => {
-            expect(chart).not.toContainReactComponent(ComparisonMetric);
-          });
-        });
+        expect(chart).not.toContainReactComponent(ComparisonMetric);
       });
 
       it('renders if comparisonMetric is provided', () => {
         const chart = mount(<DonutChart {...mockProps} />);
 
-        chart.act(() => {
-          requestAnimationFrame(() => {
-            expect(chart).toContainReactComponent(
-              ComparisonMetric,
-              mockProps.comparisonMetric,
-            );
-          });
-        });
+        expect(chart).toContainReactComponent(
+          ComparisonMetric,
+          mockProps.comparisonMetric,
+        );
       });
     });
 
@@ -141,7 +134,7 @@ describe('<DonutChart />', () => {
           />,
         );
 
-        expect(chart).not.toContainReactComponent(LegendContainer, {
+        expect(chart).toContainReactComponent(LegendContainer, {
           renderLegendContent,
         });
       });
@@ -150,11 +143,8 @@ describe('<DonutChart />', () => {
     describe('empty state', () => {
       it('renders single <Arc /> when true', () => {
         const chart = mount(<DonutChart {...mockProps} data={[]} />);
-        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('<DonutChart />', () => {
           />,
         );
 
-        chart.act(() => {
-          requestAnimationFrame(() => {
-            expect(chart).toContainReactComponentTimes(Arc, 1);
-          });
-        });
+        expect(chart).toContainReactComponentTimes(Arc, 1);
       });
 
       it('renders multiple <Arc /> when false', () => {
         const chart = mount(<DonutChart {...mockProps} />);
-        chart.act(() => {
-          requestAnimationFrame(() => {
-            expect(chart).toContainReactComponentTimes(Arc, 4);
-          });
-        });
+
+        expect(chart).toContainReactComponentTimes(Arc, 4);
+      });
+    });
+
+    describe('seriesNameFormatter', () => {
+      it('formats series name', () => {
+        const chart = mount(
+          <DonutChart
+            {...mockProps}
+            seriesNameFormatter={(name) => `series: ${name}`}
+          />,
+        );
+
+        expect(chart).toContainReactText('series: Shopify Payments');
+      });
+
+      it('does not format the series name twice', () => {
+        const chart = mount(
+          <DonutChart
+            {...mockProps}
+            seriesNameFormatter={(name) => `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
@@ -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}
           />
         </div>,
         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
@@ -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('<LegendContainer />', () => {
@@ -166,6 +160,7 @@ describe('<LegendContainer />', () => {
 
     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
@@ -26,7 +26,7 @@ describe('<Chart />', () => {
     const offsetLeft = 100;
     const offsetRight = 50;
     const margin = 2;
-    const mockWidth = 0;
+    const mockWidth = 600;
 
     mount(
       <Chart

diff --git a/...polaris-viz/src/components/TooltipContent/hooks/tests/useGetLongestLabelFromData.test.tsx b/...polaris-viz/src/components/TooltipContent/hooks/tests/useGetLongestLabelFromData.test.tsx
@@ -4,21 +4,21 @@ import {mount} from '@shopify/react-testing';
 import {useGetLongestLabelFromData} from '../useGetLongestLabelFromData';
 import type {TooltipData} from '../../../../types';
 
-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,
-      },
-    }),
-  };
-});
+// 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<any>) {
   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', () => {