Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LG-4619: Add axes components #2525

Merged
merged 8 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .changeset/pink-dingos-fetch.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
---
'@lg-charts/core': minor
---

Adds `XAxis` and `YAxis` components, improves update logic, and improves Storybook.

- Adds `XAxis` component for adding an x-axis to a chart.
- Adds `YAxis` component for adding a y-axis to a chart.
- Improves chart options update logic
- `lodash.merge` was being used originally, but this didn't work quite as expected. The goal is to merge partial options objects in without overwriting previously set options that are unrelated to the update. This was not how it worked, leading to bugs when updating the same base keys with different options. I.e. when adding splitLine configurations on xAxis in order to add grid lines, then adding more axis specific config in XAxis related to the actual axis, the splitLine would be totally overwritten.
- Improves @lg-charts/core Storybook, by adding better descriptions and organization.
28 changes: 27 additions & 1 deletion charts/core/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,12 @@ npm install @lg-charts/core
## Basic Example

```js
import { Chart, Line, Grid } from '@lg-charts/core';
import { Chart, Line, Grid, XAxis, YAxis } from '@lg-charts/core';

<Chart>
<Grid vertical={false}>
<XAxis type="time" />
<YAxis type="value" unit="GB" />
<Line
name="Series 1"
data={[
Expand Down Expand Up @@ -83,3 +85,27 @@ Component that displays grid lines on the chart.
| ------------ | --------------------------- | ------- | ------- |
| `horizontal` | Show horizontal grid lines. | boolean | true |
| `vertical` | Show vertical grid lines. | boolean | true |

### `XAxis`

Renders an x-axis.

#### Props

| Name | Description | Type | Default |
| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------- | ------- |
| `type` | Type of axis. | 'category' \| 'value' \| 'time' \| 'log' | |
| `label` _(optional)_ | Label name to be rendered on the axis. | string | |
| `unit` _(optional)_ | String that will be appended to the values of the axis. Only applies if `type` of `value`. _Note: this unit will not impact the data. E.g. if data is given in MB and the units are set to "GB", the component won’t convert these values. Conversion of data is up to the consumer._ | string | |

### `YAxis`

Renders a y-axis.

#### Props

| Name | Description | Type | Default |
| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------- | ------- |
| `type` | Type of axis. | 'category' \| 'value' \| 'time' \| 'log' | |
| `label` _(optional)_ | Label name to be rendered on the axis. | string | |
| `unit` _(optional)_ | String that will be appended to the values of the axis. Only applies if `type` of `value`. _Note: this unit will not impact the data. E.g. if data is given in MB and the units are set to "GB", the component won’t convert these values. Conversion of data is up to the consumer._ | string | |
6 changes: 2 additions & 4 deletions charts/core/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,14 @@
"@leafygreen-ui/tokens": "^2.11.0",
"@lg-tools/storybook-utils": "^0.1.1",
"echarts": "^5.5.1",
"lodash.debounce": "^4.0.8",
"lodash.merge": "^4.6.2"
"lodash.debounce": "^4.0.8"
},
"peerDependencies": {
"@leafygreen-ui/leafygreen-provider": "^3.1.12"
},
"devDependencies": {
"@faker-js/faker": "8.0.2",
"@types/lodash.debounce": "^4.0.9",
"@types/lodash.merge": "^4.6.9"
"@types/lodash.debounce": "^4.0.9"
},
"repository": {
"type": "git",
Expand Down
2 changes: 1 addition & 1 deletion charts/core/src/Chart/Chart.styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export const getWrapperStyles = (theme: Theme) => css`
height: 280px;
width: 100%;
border: 1px solid
${color[theme].border[Variant.Secondary][InteractionState.Default]};
${color[theme].border[Variant.Disabled][InteractionState.Default]};
border-radius: ${borderRadius[200]}px;
display: grid;
grid-template-rows: auto 1fr;
Expand Down
20 changes: 14 additions & 6 deletions charts/core/src/Chart/hooks/updateUtils.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,30 @@ describe('@lg-charts/core/Chart/hooks/updateUtils', () => {
expect(updatedOptions.series?.[0].name).toBe(seriesName2);
});

test('should update the chart options', () => {
/**
* Tests that option updates don't overwrite the entire chart options object.
*/
test('should merge chart options non-destructively', () => {
const currentOptions: Partial<ChartOptions> = {
aria: {
xAxis: {
show: true,
},
grid: {
show: false,
splitLine: {
show: true,
},
},
};
const updatedOptions = updateOptions(currentOptions, {
xAxis: {
show: false, // This should only update the show property and not other properties
},
grid: {
show: true,
},
});
// @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
expect(updatedOptions?.aria?.show).toBe(true);
expect(updatedOptions?.xAxis?.show).toBe(false);
// @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
expect(updatedOptions?.xAxis?.splitLine?.show).toBe(true);
// @ts-ignore: Property 'show' does not exist on type 'Arrayable<GridOption>'.
expect(updatedOptions?.grid?.show).toBe(true);
});
Expand Down
33 changes: 30 additions & 3 deletions charts/core/src/Chart/hooks/updateUtils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import merge from 'lodash.merge';

import { ChartOptions, SeriesOption } from '../Chart.types';

export function addSeries(
Expand Down Expand Up @@ -32,9 +30,38 @@ export function removeSeries(
return updatedOptions;
}

/**
* Method to recursively merge two objects. It should update keys if they
* already exist and add them if they don't. However, it shouldn't completely
* overwrite a key it's an already existing object.
*
* They goal is to allow for partial updates to the chart options object.
*/
function recursiveMerge(
target: { [key: string]: any },
source: { [key: string]: any },
) {
const updatedObj = { ...target };

for (const key in source) {
if (
typeof source[key] === 'object' &&
typeof updatedObj[key] === 'object'
) {
// Recursively update nested objects
updatedObj[key] = recursiveMerge(updatedObj[key], source[key]);
} else {
// Update or add the value for the key
updatedObj[key] = source[key];
}
}

return updatedObj;
}

export function updateOptions(
currentOptions: ChartOptions,
options: Partial<ChartOptions>,
): Partial<ChartOptions> {
return merge({ ...currentOptions, ...options });
return recursiveMerge(currentOptions, options);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why we're replacing the lodash merge, but that's a question not a blocker

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to merge partial options objects in without overwriting previously set options that are unrelated to the update. This wasn't actually how lodash.merge worked (which I thought it would), leading to bugs when updating the same base keys with different options. I.e. when adding splitLine configurations on xAxis in order to add grid lines, then adding more axis specific config in the XAxis component related to the actual axis, the splitLine would be totally overwritten.

The updated test is a good example of what should pass and wasn't with lodash.merge:

  /**
   * Tests that option updates don't overwrite the entire chart options object.
   */
  test('should merge chart options non-destructively', () => {
    const currentOptions: Partial<ChartOptions> = {
      xAxis: {
        show: true,
        splitLine: {
          show: true,
        },
      },
    };
    const updatedOptions = updateOptions(currentOptions, {
      xAxis: {
        show: false, // This should only update the show property and not other properties
      },
      grid: {
        show: true,
      },
    });
    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
    expect(updatedOptions?.xAxis?.show).toBe(false);
    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<AriaOption>'.
    expect(updatedOptions?.xAxis?.splitLine?.show).toBe(true);
    // @ts-ignore: Property 'show' does not exist on type 'Arrayable<GridOption>'.
    expect(updatedOptions?.grid?.show).toBe(true);
  });

2 changes: 2 additions & 0 deletions charts/core/src/Chart/hooks/useChart.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,12 +81,14 @@ describe('@lg-echarts/core/hooks/useChart', () => {
const { result } = renderHook(() => useChart({ theme: 'dark' }));
const options = {
title: {
show: true,
text: 'test',
},
};
act(() => {
result.current.updateChartOptions(options);
});

await waitForState(() => {
expect(result.current.chartOptions.title).toEqual(options.title);
});
Expand Down
65 changes: 38 additions & 27 deletions charts/core/src/Chart/hooks/useChart.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState } from 'react';
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { LineChart as EchartsLineChart } from 'echarts/charts';
import {
GridComponent,
Expand Down Expand Up @@ -71,35 +71,46 @@ export function useChart({ theme, onChartReady }: ChartHookProps) {
};
}, []);

const updateChartRef = debounce((chartOptions: Partial<ChartOptions>) => {
chartInstanceRef.current?.setOption(chartOptions);
}, 50);
const updateChartRef = useMemo(
() =>
debounce((chartOptions: Partial<ChartOptions>) => {
chartInstanceRef.current?.setOption(chartOptions);
}, 50),
[],
);

const addChartSeries = (data: SeriesOption) => {
setChartOptions(currentOptions => {
const updatedOptions = addSeries(currentOptions, data);
updateChartRef(updatedOptions);
return updatedOptions;
});
};
const addChartSeries = useCallback(
(data: SeriesOption) => {
setChartOptions(currentOptions => {
const updatedOptions = addSeries(currentOptions, data);
updateChartRef(updatedOptions);
return updatedOptions;
});
},
[updateChartRef],
);

const removeChartSeries = (name: string) => {
setChartOptions(currentOptions => {
const updatedOptions = removeSeries(currentOptions, name);
updateChartRef(updatedOptions);
return updatedOptions;
});
};
const removeChartSeries = useCallback(
(name: string) => {
setChartOptions(currentOptions => {
const updatedOptions = removeSeries(currentOptions, name);
updateChartRef(updatedOptions);
return updatedOptions;
});
},
[updateChartRef],
);

const updateChartOptions = (
options: Omit<Partial<ChartOptions>, 'series'>,
) => {
setChartOptions(currentOptions => {
const updatedOptions = updateOptions(currentOptions, options);
updateChartRef(updatedOptions);
return updatedOptions;
});
};
const updateChartOptions = useCallback(
(options: Omit<Partial<ChartOptions>, 'series'>) => {
setChartOptions(currentOptions => {
const updatedOptions = updateOptions(currentOptions, options);
updateChartRef(updatedOptions);
return updatedOptions;
});
},
[updateChartRef],
);

useEffect(() => {
setChartOptions(currentOptions => {
Expand Down
103 changes: 103 additions & 0 deletions charts/core/src/XAxis/XAxis.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import { useEffect } from 'react';

import { useDarkMode } from '@leafygreen-ui/leafygreen-provider';
import { Theme } from '@leafygreen-ui/lib';
import {
color,
fontFamilies,
fontWeights,
InteractionState,
spacing,
Variant,
} from '@leafygreen-ui/tokens';

import { ChartOptions } from '../Chart/Chart.types';
import { useChartContext } from '../ChartContext';

import { XAxisProps, XAxisType } from './XAxis.types';

const getOptions = ({
theme,
type,
label,
unit,
}: XAxisProps & { theme: Theme }): Partial<ChartOptions> => {
const options: Partial<ChartOptions> = {
xAxis: {
type: type,
axisLine: {
show: true,
lineStyle: {
color:
color[theme].border[Variant.Secondary][InteractionState.Default],
width: 1,
},
},
axisLabel: {
show: true,
fontFamily: fontFamilies.default,
fontWeight: fontWeights.medium,
fontSize: 11,
lineHeight: spacing[400],
color: color[theme].text[Variant.Secondary][InteractionState.Default],
align: 'center',
margin: spacing[400],
formatter:
unit && type === XAxisType.Value
? (value: string) => `${value}${unit}`
: undefined,
},
axisTick: {
show: false,
},
name: label,
nameLocation: 'middle',
nameTextStyle: {
fontFamily: fontFamilies.default,
fontWeight: fontWeights.medium,
fontSize: 11,
lineHeight: spacing[400],
padding: [spacing[200], 0, 0, 0],
color: color[theme].text[Variant.Secondary][InteractionState.Default],
},
nameGap: spacing[1000],
},
};

if (label) {
options.grid = {
bottom: spacing[1200], // Pushes out to make room for the label
};
} else {
options.grid = {
bottom: spacing[400], // Default bottom spacing
};
}

return options;
};

/**
* React component that can render an x-axis on a parent chart.
*
* This is done by updating the parent chart's canvas configuration received via context.
*
* ```
* <Chart>
* <XAxis
* type="time",
* label="My X-Axis Data",
* unit="GB"
* />
* </Chart>
*/
export function XAxis({ type, label, unit }: XAxisProps) {
const { updateChartOptions } = useChartContext();
const { theme } = useDarkMode();

useEffect(() => {
updateChartOptions(getOptions({ type, label, unit, theme }));
}, [type, label, unit, theme, updateChartOptions]);

return null;
}
24 changes: 24 additions & 0 deletions charts/core/src/XAxis/XAxis.types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
export const XAxisType = {
Category: 'category',
Value: 'value',
Time: 'time',
Log: 'log',
} as const;
type XAxisType = (typeof XAxisType)[keyof typeof XAxisType];

export interface XAxisProps {
/**
* Type of axis.
*/
type: XAxisType;

/**
* Label name of the axis.
*/
label?: string;

/**
* Unit of the axis to be rendered with value.
*/
unit?: string;
}
2 changes: 2 additions & 0 deletions charts/core/src/XAxis/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export { XAxis } from './XAxis';
export { XAxisProps } from './XAxis.types';
Loading
Loading