fix(heatmap): fix code review feedbacks

chanzuckerberg · Feb 14, 2024 · ae9daa5 · ae9daa5
1 parent f9c6feb
commit ae9daa5
Show file tree

Hide file tree

Showing 8 changed files with 402 additions and 382 deletions.
diff --git a/package.json b/package.json
@@ -26,22 +26,22 @@
     "@reduxjs/toolkit": "^2.1.0",
     "@rollup/plugin-typescript": "^11.1.5",
     "@rollup/plugin-url": "^8.0.2",
-    "@storybook/addon-a11y": "^7.6.13",
-    "@storybook/addon-actions": "^7.6.13",
-    "@storybook/addon-essentials": "^7.6.13",
-    "@storybook/addon-interactions": "^7.6.13",
-    "@storybook/addon-links": "^7.6.13",
-    "@storybook/addon-storyshots": "^7.6.13",
-    "@storybook/addons": "^7.6.13",
-    "@storybook/components": "^7.6.13",
-    "@storybook/core-events": "^7.6.13",
-    "@storybook/manager-api": "^7.6.13",
-    "@storybook/preview-api": "^7.6.13",
-    "@storybook/react": "^7.6.13",
-    "@storybook/react-webpack5": "^7.6.13",
+    "@storybook/addon-a11y": "^7.6.15",
+    "@storybook/addon-actions": "^7.6.15",
+    "@storybook/addon-essentials": "^7.6.15",
+    "@storybook/addon-interactions": "^7.6.15",
+    "@storybook/addon-links": "^7.6.15",
+    "@storybook/addon-storyshots": "^7.6.15",
+    "@storybook/addons": "^7.6.15",
+    "@storybook/components": "^7.6.15",
+    "@storybook/core-events": "^7.6.15",
+    "@storybook/manager-api": "^7.6.15",
+    "@storybook/preview-api": "^7.6.15",
+    "@storybook/react": "^7.6.15",
+    "@storybook/react-webpack5": "^7.6.15",
     "@storybook/test-runner": "^0.16.0",
     "@storybook/testing-library": "^0.2.2",
-    "@storybook/theming": "^7.6.13",
+    "@storybook/theming": "^7.6.15",
     "@svgr/rollup": "^8.1.0",
     "@svgr/webpack": "^8.1.0",
     "@testing-library/dom": "^9.3.3",
@@ -113,7 +113,7 @@
     "sass": "^1.70.0",
     "sass-loader": "^14.1.0",
     "simplex-noise": "^4.0.1",
-    "storybook": "^7.6.13",
+    "storybook": "^7.6.15",
     "storybook-addon-emotion-theme": "^2.1.1",
     "storybook-addon-pseudo-states": "^2.1.2",
     "style-dictionary": "^3.9.2",

diff --git a/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/ECharts/index.tsx b/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/ECharts/index.tsx
@@ -319,10 +319,6 @@ const ECharts = forwardRef(
               type: "downplay",
             });
 
-          // const data = echartInstance
-          //   ? (echartInstance.getOption().series as any)[0].data
-          //   : [];
-
           const dataIndex = [];
 
           switch (emphasis) {
@@ -374,9 +370,6 @@ const ECharts = forwardRef(
               type: "downplay",
             });
 
-          // const data = echartInstance
-          //   ? (echartInstance.getOption().series as any)[0].data
-          //   : [];
           const dataIndex = [];
 
           for (let i = 0; i < size; i++) {
@@ -401,10 +394,6 @@ const ECharts = forwardRef(
               type: "downplay",
             });
 
-          // const data = echartInstance
-          //   ? (echartInstance.getOption().series as any)[0].data
-          //   : [];
-
           const dataIndex = [];
 
           for (let i = 0; i < size; i++) {

diff --git a/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/Heatmap/index.tsx b/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/Heatmap/index.tsx
@@ -37,7 +37,7 @@ const Heatmap = () => {
   const chartRef = useRef<HTMLDivElement>(null);
 
   return (
-    <StyledContainer camera={camera}>
+    <StyledContainer>
       <YAxisWrapper
         id="y-axis-wrapper"
         height={camera ? heatmapCanvasSize.height : HEATMAP_ITEM_SIZE * size}

diff --git a/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/Heatmap/style.ts b/packages/data-viz/src/core/HeatmapChart/heatmapDemo/components/Heatmap/style.ts
@@ -1,25 +1,13 @@
 import { styled } from "@mui/material";
 import { X_AXIS_WIDTH, Y_AXIS_WIDTH } from "../utils";
 
-interface StylesProps {
-  camera?: boolean;
-}
-
 const doNotForwardProps = ["camera"];
 
 export const StyledContainer = styled("div", {
   shouldForwardProp: (prop: string) => !doNotForwardProps.includes(prop),
 })`
-  ${(props: StylesProps) => {
-    const { camera } = props;
-
-    return `
-      margin-bottom: ${camera ? 0 : 30}px;
-      margin-right: ${camera ? 0 : 20}px;
-      padding-top: ${X_AXIS_WIDTH}px;
-      position: relative;
-    `;
-  }}
+  padding-top: ${X_AXIS_WIDTH}px;
+  position: relative;
 `;
 
 export const StyledHeatmapWrapper = styled("div")`

diff --git a/packages/data-viz/src/core/HeatmapChart/hooks/utils.ts b/packages/data-viz/src/core/HeatmapChart/hooks/utils.ts
@@ -1,4 +1,5 @@
 import {
+  AxisPointerComponentOption,
   DataZoomComponentOption,
   DatasetComponentOption,
   ECharts,
@@ -189,7 +190,7 @@ export function createChartOptions(
 
   return {
     animation: false,
-    axisPointer: Object.assign(
+    axisPointer: mergeAxisPointer(
       defaultAxisPointer,
       axisPointer,
       optionsAxisPointer
@@ -243,6 +244,36 @@ export function createChartOptions(
   };
 }
 
+function mergeAxisPointer(
+  defaultAxisPointer: AxisPointerComponentOption,
+  axisPointer: EChartsOption["axisPointer"],
+  optionsAxisPointer: EChartsOption["axisPointer"]
+): EChartsOption["axisPointer"] {
+  const finalAxisPointer = Array.isArray(axisPointer)
+    ? axisPointer // if it's an array, assume that the user has [{...propsToChangeOnX}] OR [ , {...propsToChangeOnY}] OR [{...propsToChangeOnX}, {...propsToChangeOnY}]
+    : [axisPointer, axisPointer]; // else if the user only supplies one dataZoom object, we assume they want the props to apply to both x and y zoom objects
+
+  const finalOptionsAxisPointer = Array.isArray(optionsAxisPointer)
+    ? optionsAxisPointer
+    : [optionsAxisPointer, optionsAxisPointer];
+
+  // merge x axisPointer options
+  const x = {
+    ...defaultAxisPointer,
+    ...finalAxisPointer[0],
+    ...finalOptionsAxisPointer?.[0],
+  };
+
+  // merge y axisPointer options
+  const y = {
+    ...defaultAxisPointer,
+    ...finalAxisPointer[1],
+    ...finalOptionsAxisPointer[1],
+  };
+
+  return [x, y] as EChartsOption["axisPointer"];
+}
+
 function mergeDataZoom(
   defaultDataZoom: DataZoomComponentOption[],
   dataZoom: EChartsOption["dataZoom"],
@@ -289,7 +320,7 @@ function generateDefaultValues(props: CreateChartOptionsProps) {
     label: { show: false },
     show: false,
     triggerOn: "mousemove",
-  };
+  } as AxisPointerComponentOption;
 
   const defaultXAxis = {
     axisLabel: { fontSize: 0, rotate: 90 },

diff --git a/packages/data-viz/src/core/HeatmapChart/index.tsx b/packages/data-viz/src/core/HeatmapChart/index.tsx
@@ -10,7 +10,7 @@ import {
   useState,
 } from "react";
 
-import { EMPTY_OBJECT } from "src/common/utils";
+import { EMPTY_OBJECT } from "../../common/utils";
 import { useUpdateChart } from "./hooks/useUpdateChart";
 import { CreateChartOptionsProps } from "./hooks/utils";
 import { ChartContainer } from "./style";
@@ -107,20 +107,32 @@ const HeatmapChart = forwardRef(
           }
         }
       };
-      // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [chart, echartsRendererMode, isChartInitialized, onEvents]);
 
     // Initialize charts on component mount
     useEffect(() => {
       initChart();
     }, [initChart]);
 
-    // Reinitialize chart if renderer mode changes
+    // Reinitialize the chart when the renderer mode changes
     useEffect(() => {
+      // Dispose of the current chart
       chart?.dispose();
+
+      // Reset chart initialization state and set chart to null
       setIsChartInitialized(false);
       setChart(null);
+
+      // Initialize a new chart
       initChart();
+
+      /**
+       * (masoudmanson): We don't include the 'chart' dependency in the array because
+       * doing so would lead to an infinite loop of initializing and disposing the chart.
+       * We specifically want to reinitialize the chart only when the renderer mode changes.
+       * However, since we use the 'chart' state to dispose of the chart, we disable the
+       * eslint rule for exhaustive-deps to prevent warnings.
+       */
       // eslint-disable-next-line react-hooks/exhaustive-deps
     }, [echartsRendererMode]);
 

diff --git a/tsconfig.json b/tsconfig.json
@@ -59,10 +59,10 @@
     "skipLibCheck": true
   },
   "include": [
-    "packages/*/src/**/*",
+    "./packages/*/src/**/*",
     ".eslintrc.js"
   ],
-  "exclude": ["node_modules", "packages/*/node_modules"],
+  "exclude": ["node_modules", "./packages/*/node_modules"],
   "references": [
     { "path": "./packages/components" },
     { "path": "./packages/data-viz" }