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.

Sign up for GitHub

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

Jump to bottom

LG-4659: Replace axes unit prop with formatter #2536

Merged
merged 7 commits into from
Nov 4, 2024
+64 −55
Merged

LG-4659: Replace axes unit prop with formatter #2536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
4d7d8c8
Replace `unit` with `formatter`
tsck Nov 1, 2024
f02eee9
Update README
tsck Nov 1, 2024
3902ebe
Changeset
tsck Nov 1, 2024
cf3a4ed
Add formatting to time in storybook
tsck Nov 1, 2024
3734ae7
Merge branch 'main' of github.com:mongodb/leafygreen-ui into LG-4659
tsck Nov 1, 2024
ac6258a
Remove documented support for non-callback formatter types
tsck Nov 1, 2024
5447e1b
Update README example
tsck Nov 1, 2024
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
5 changes: 5 additions & 0 deletions .changeset/silly-jars-cheat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lg-charts/core': minor
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess this is technically major since it's not backwards compatible, but since it's still in v0, I made it minor. Let me know if this is incorrect, though!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, pre-v1 changesets can be a bit loose. Rule of thumb for me is everything shifts down a level

---

Replaces `unit` prop of `XAxis` and `YAxis` with `formatter` prop
22 changes: 11 additions & 11 deletions charts/core/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { Chart, Line, Grid, XAxis, YAxis } from '@lg-charts/core';
<Chart>
<Grid vertical={false}>
<XAxis type="time" />
<YAxis type="value" unit="GB" />
<YAxis type="value" formatter={(value) => `${value}GB`} />
<Line
name="Series 1"
data={[
Expand Down Expand Up @@ -92,20 +92,20 @@ 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 | |
| Name | Description | Type | Default |
| ------------------------ | --------------------------------------------- | ---------------------------------------- | ------- |
| `type` | Type of axis. | 'category' \| 'value' \| 'time' \| 'log' | |
| `label` _(optional)_ | Label name to be rendered on the axis. | string | |
| `formatter` _(optional)_ | Callback function for formatting tick values. | (value: string, index: number) => 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 | |
| Name | Description | Type | Default |
| ------------------------ | --------------------------------------------- | ---------------------------------------- | ------- |
| `type` | Type of axis. | 'category' \| 'value' \| 'time' \| 'log' | |
| `label` _(optional)_ | Label name to be rendered on the axis. | string | |
| `formatter` _(optional)_ | Callback function for formatting tick values. | (value: string, index: number) => string | |
17 changes: 7 additions & 10 deletions charts/core/src/XAxis/XAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import {
import { ChartOptions } from '../Chart/Chart.types';
import { useChartContext } from '../ChartContext';

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

const getOptions = ({
theme,
type,
label,
unit,
formatter,
}: XAxisProps & { theme: Theme }): Partial<ChartOptions> => {
const options: Partial<ChartOptions> = {
xAxis: {
Expand All @@ -42,10 +42,7 @@ const getOptions = ({
color: color[theme].text[Variant.Secondary][InteractionState.Default],
align: 'center',
margin: spacing[400],
formatter:
unit && type === XAxisType.Value
? (value: string) => `${value}${unit}`
: undefined,
formatter,
},
axisTick: {
show: false,
Expand Down Expand Up @@ -96,16 +93,16 @@ const unsetAxisOptions = {
* <XAxis
* type="time",
* label="My X-Axis Data",
* unit="GB"
* formatter="{value}GB"
Copy link
Collaborator

@TheSonOfThomp TheSonOfThomp Nov 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

is this correct? Oh I see now. I was expecting a template literal. This is how ECharts interpolates strings?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, exactly. Specifically for rendering the value. There are other formatters such as '{yyyy}-{MM}-{dd}' as well.

* />
* </Chart>
*/
export function XAxis({ type, label, unit }: XAxisProps) {
export function XAxis({ type, label, formatter }: XAxisProps) {
const { updateChartOptions } = useChartContext();
const { theme } = useDarkMode();

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

return () => {
/**
Expand All @@ -115,7 +112,7 @@ export function XAxis({ type, label, unit }: XAxisProps) {
xAxis: unsetAxisOptions,
});
};
}, [type, label, unit, theme, updateChartOptions]);
}, [type, label, formatter, theme, updateChartOptions]);

return null;
}
9 changes: 7 additions & 2 deletions charts/core/src/XAxis/XAxis.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@ export interface XAxisProps {
label?: string;

/**
* Unit of the axis to be rendered with value.
*
* Formatter of axis label, which supports string template and callback function.
*
* ```ts
* formatter: (value, index) => `${value}GB`
* ```
*/
unit?: string;
formatter?: (value: string, index: number) => string;
}
17 changes: 7 additions & 10 deletions charts/core/src/YAxis/YAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,13 @@ import {
import { ChartOptions } from '../Chart/Chart.types';
import { useChartContext } from '../ChartContext';

import { YAxisProps, YAxisType } from './YAxis.types';
import { YAxisProps } from './YAxis.types';

const getOptions = ({
theme,
type,
label,
unit,
formatter,
}: YAxisProps & { theme: Theme }): Partial<ChartOptions> => {
const options: Partial<ChartOptions> = {
yAxis: {
Expand All @@ -42,10 +42,7 @@ const getOptions = ({
color: color[theme].text[Variant.Secondary][InteractionState.Default],
align: 'right',
margin: spacing[200],
formatter:
unit && type === YAxisType.Value
? (value: string) => `${value}${unit}`
: undefined,
formatter,
},
axisTick: {
show: false,
Expand Down Expand Up @@ -95,16 +92,16 @@ const unsetAxisOptions = {
* <YAxis
* type="value",
* label="My Y-Axis Data",
* unit="GB"
* formatter="{value}GB"
* />
* </Chart>
*/
export function YAxis({ type, label, unit }: YAxisProps) {
export function YAxis({ type, label, formatter }: YAxisProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like there's a lot of similarity between XAxis & YAxis. Not necessary in this PR, but could be worth DRY-ing this up a bit

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup, they're almost identical atm with just a few different props. I considered having a more common component that these two work off of but decided not to since there are only 2, but I agree, it probably makes sense. I'll move in this direction in a future PR.

const { updateChartOptions } = useChartContext();
const { theme } = useDarkMode();

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

return () => {
/**
Expand All @@ -114,7 +111,7 @@ export function YAxis({ type, label, unit }: YAxisProps) {
yAxis: unsetAxisOptions,
});
};
}, [type, label, unit, theme, updateChartOptions]);
}, [type, label, formatter, theme, updateChartOptions]);

return null;
}
12 changes: 10 additions & 2 deletions charts/core/src/YAxis/YAxis.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ export interface YAxisProps {
label?: string;

/**
* Unit of the axis to be rendered with value. Only applies if `type` of `value`.
*
* Formatter of axis label, which supports string template and callback function.
*
* ```ts
* // Use callback.
* formatter: function (value, index) {
* return value + 'kg';
*}
* ```
*/
unit?: string;
formatter?: (value: string, index: number) => string;
}
37 changes: 17 additions & 20 deletions charts/core/src/core.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,11 @@ export default {
category: 'XAxis',
},
},
xAxisUnit: {
control: 'text',
description: 'X-axis units',
name: 'Unit',
xAxisFormatter: {
description: 'X-axis formatter',
name: 'Formatter',
table: {
category: 'XAxis',
disable: true,
},
},
xAxisLabel: {
Expand All @@ -68,12 +67,11 @@ export default {
category: 'YAxis',
},
},
yAxisUnit: {
control: 'text',
description: 'Y-axis units',
name: 'Unit',
yAxisFormatter: {
description: 'Y-axis formatter',
name: 'Formatter',
table: {
category: 'YAxis',
disable: true,
},
},
yAxisLabel: {
Expand Down Expand Up @@ -103,10 +101,10 @@ interface StoryChartProps {
horizontalGridLines: boolean;
xAxisType: XAxisProps['type'];
yAxisType: YAxisProps['type'];
xAxisUnit: string;
yAxisUnit: string;
xAxisLabel: string;
yAxisLabel: string;
xAxisFormatter: XAxisProps['formatter'];
yAxisFormatter: XAxisProps['formatter'];
xAxisLabel: XAxisProps['label'];
yAxisLabel: YAxisProps['label'];
}

const Template: React.FC<StoryChartProps> = props => {
Expand All @@ -115,18 +113,18 @@ const Template: React.FC<StoryChartProps> = props => {
verticalGridLines,
horizontalGridLines,
xAxisType,
xAxisUnit,
xAxisFormatter,
yAxisType,
yAxisUnit,
yAxisFormatter,
xAxisLabel,
yAxisLabel,
} = props;

return (
<Chart {...props}>
<Grid vertical={verticalGridLines} horizontal={horizontalGridLines} />
<XAxis type={xAxisType} unit={xAxisUnit} label={xAxisLabel} />
<YAxis type={yAxisType} unit={yAxisUnit} label={yAxisLabel} />
<XAxis type={xAxisType} formatter={xAxisFormatter} label={xAxisLabel} />
<YAxis type={yAxisType} formatter={yAxisFormatter} label={yAxisLabel} />
{data.map(({ name, data }) => (
<Line name={name} data={data} key={name} />
))}
Expand All @@ -141,6 +139,5 @@ LiveExample.args = {
verticalGridLines: false,
xAxisType: 'time',
yAxisType: 'value',
xAxisUnit: '',
yAxisUnit: 'GB',
yAxisFormatter: value => `${value}GB`,
};
Loading