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
Merged

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

Show file tree
Hide file tree
Changes from 4 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}GB" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

this api does feel a little tied to echarts - should we leverage a more expected API and then normalize it on our side before sending thru

Copy link
Collaborator

Choose a reason for hiding this comment

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

Certainly open to discussion here, just wanted to call it out

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed! I'm thinking that we just publicly expose the callback. Addressing this more in this thread

Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome

<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)_ | Formatter of axis label, which supports string template and callback function. (See [ECharts xAxis.axisLabel.formatter docs](https://echarts.apache.org/en/option.html#xAxis.axisLabel.formatter) for more details) | string \| ((value: string, index: number) => string) \| { [key: string]: string } | |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it weird to link out to the ECharts docs in our docs? 🤔

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.

Not strange to link out, but we might want to be careful with the wording. Do we want to commit ourselves to sticking with the ECharts API, or do we want to give ourselves more freedom? (I think this is fine, but just want to be careful about implying that our API is exactly the same as theirs)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call out - agreed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressing this more in thread here (TL;DR - I think this probably should be removed, and we should only publicly support a subset for this)


### `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)_ | Formatter of axis label, which supports string template and callback function. (See [ECharts yAxis.axisLabel.formatter docs](https://echarts.apache.org/en/option.html#yAxis.axisLabel.formatter) for more details) | string \| ((value: string, index: number) => string) \| { [key: string]: 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 @@ -87,17 +84,17 @@ const getOptions = ({
* <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 }));
}, [type, label, unit, theme, updateChartOptions]);
updateChartOptions(getOptions({ type, label, formatter, theme }));
}, [type, label, formatter, theme, updateChartOptions]);

return null;
}
18 changes: 16 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,21 @@ 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
* // Use string template; template variable is the default label of axis {value}
* formatter: '{value} kg'
*
* // Use callback.
* formatter: function (value, index) {
* return value + 'kg';
*}
* ```
*/
unit?: string;
formatter?:
| string
| ((value: string, index: number) => string)
| { [key: string]: string };
Copy link
Collaborator

Choose a reason for hiding this comment

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

What would an object formatter look like?

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 object formatter is what echarts calls a "cascading template" and only works with time. From their site:

Sometimes, we wish to use different formats for different time granularity. For example, in a quarter-year chart, we may wish to see the month name with the first date of the month, while see the date name with others. This can be made with:

formatter: {
   month: '{MMMM}', // Jan, Feb, ...
   day: '{d}' // 1, 2, ...
}

I guess that means this could actually be typed better:

Suggested change
| { [key: string]: string };
| { [key: CascadingTemplateLevels]: string };

where CascadingTemplateLevels is defined as:

type CascadingTemplateLevels =
  | 'year'
  | 'month'
  | 'day'
  | 'hour'
  | 'minute'
  | 'second'
  | 'millisecond'
  | 'none';

However, I think given your point in this comment, I'm wondering if we should publicly support this. Given that EChart's string interpolation isn't standard JS either, I'm almost wondering if we should just publicly support the callback function. Consumers can still get hacky, override the type and use either of these, but by not publicly supporting them it probably helps save some headaches in the future if we ever need to switch the underlying library. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Decided to make the decision to only support the callback function and have pushed changes to reflect that. Let me know if there's any disagreement here.

}
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 @@ -86,17 +83,17 @@ const getOptions = ({
* <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 }));
}, [type, label, unit, theme, updateChartOptions]);
updateChartOptions(getOptions({ type, label, formatter, theme }));
}, [type, label, formatter, theme, updateChartOptions]);

return null;
}
18 changes: 16 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,21 @@ 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 string template; template variable is the default label of axis {value}
* formatter: '{value} kg'
*
* // Use callback.
* formatter: function (value, index) {
* return value + 'kg';
*}
* ```
*/
unit?: string;
formatter?:
| string
| ((value: string, index: number) => string)
| { [key: string]: string };
}
28 changes: 14 additions & 14 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,10 +43,10 @@ export default {
category: 'XAxis',
},
},
xAxisUnit: {
xAxisFormatter: {
control: 'text',
description: 'X-axis units',
name: 'Unit',
description: 'X-axis formatter',
name: 'Formatter',
table: {
category: 'XAxis',
},
Expand All @@ -68,10 +68,10 @@ export default {
category: 'YAxis',
},
},
yAxisUnit: {
yAxisFormatter: {
control: 'text',
description: 'Y-axis units',
name: 'Unit',
description: 'Y-axis formatter',
name: 'Formatter',
table: {
category: 'YAxis',
},
Expand Down Expand Up @@ -103,8 +103,8 @@ interface StoryChartProps {
horizontalGridLines: boolean;
xAxisType: XAxisProps['type'];
yAxisType: YAxisProps['type'];
xAxisUnit: string;
yAxisUnit: string;
xAxisFormatter: string;
yAxisFormatter: string;
xAxisLabel: string;
yAxisLabel: string;
}
Expand All @@ -115,18 +115,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 +141,6 @@ LiveExample.args = {
verticalGridLines: false,
xAxisType: 'time',
yAxisType: 'value',
xAxisUnit: '',
yAxisUnit: 'GB',
xAxisFormatter: '{hh}:{mm}',
yAxisFormatter: '{value}GB',
};
Loading