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

[charts] Remove colors prop from SparkLineChart. #16494

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
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
4 changes: 2 additions & 2 deletions docs/data/charts/sparkline/CustomYAxis.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export default function CustomYAxis() {
<Typography>Without fixed y-range</Typography>
<Stack sx={{ width: '100%', mb: 2 }} direction="row" spacing={2}>
<Box sx={{ flexGrow: 1 }}>
<SparkLineChart data={smallValues} colors={['red']} {...settings} />
<SparkLineChart data={smallValues} color="red" {...settings} />
</Box>
<Box sx={{ flexGrow: 1 }}>
<SparkLineChart data={largeValues} {...settings} />
Expand All @@ -34,7 +34,7 @@ export default function CustomYAxis() {
min: 0,
max: 100,
}}
colors={['red']}
color="red"
{...settings}
/>
</Box>
Expand Down
4 changes: 2 additions & 2 deletions docs/data/charts/sparkline/CustomYAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export default function CustomYAxis() {
<Typography>Without fixed y-range</Typography>
<Stack sx={{ width: '100%', mb: 2 }} direction="row" spacing={2}>
<Box sx={{ flexGrow: 1 }}>
<SparkLineChart data={smallValues} colors={['red']} {...settings} />
<SparkLineChart data={smallValues} color="red" {...settings} />
</Box>
<Box sx={{ flexGrow: 1 }}>
<SparkLineChart data={largeValues} {...settings} />
Expand All @@ -34,7 +34,7 @@ export default function CustomYAxis() {
min: 0,
max: 100,
}}
colors={['red']}
color="red"
{...settings}
/>
</Box>
Expand Down
6 changes: 0 additions & 6 deletions docs/pages/x/api/charts/spark-line-chart.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,6 @@
"type": { "name": "union", "description": "func<br>&#124;&nbsp;string" },
"default": "rainbowSurgePalette[0]"
},
"colors": {
"type": { "name": "union", "description": "Array&lt;string&gt;<br>&#124;&nbsp;func" },
"default": "rainbowSurgePalette",
"deprecated": true,
"deprecationInfo": "use the <code>color</code> prop instead"
},
"dataset": { "type": { "name": "arrayOf", "description": "Array&lt;object&gt;" } },
"disableAxisListener": { "type": { "name": "bool" }, "default": "false" },
"height": { "type": { "name": "number" } },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"description": "Set to <code>true</code> to fill spark line area. Has no effect if plotType=&#39;bar&#39;."
},
"color": { "description": "Color used to colorize the sparkline." },
"colors": { "description": "Color palette used to colorize multiple series." },
"data": { "description": "Data to plot." },
"dataset": {
"description": "An array of objects that can be used to populate series and axes data using their <code>dataKey</code> property."
Expand Down
21 changes: 5 additions & 16 deletions packages/x-charts/src/SparkLineChart/SparkLineChart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export interface SparkLineChartSlotProps
ChartsTooltipSlotProps {}

export interface SparkLineChartProps
extends Omit<ChartContainerProps, 'series' | 'xAxis' | 'yAxis' | 'zAxis' | 'margin' | 'plugins'> {
extends Omit<
ChartContainerProps,
'series' | 'xAxis' | 'yAxis' | 'zAxis' | 'margin' | 'plugins' | 'colors'
> {
/**
* The xAxis configuration.
* Notice it is a single [[AxisConfig]] object, not an array of configuration.
Expand Down Expand Up @@ -108,13 +111,6 @@ export interface SparkLineChartProps
*/
slotProps?: SparkLineChartSlotProps;

/**
* Color palette used to colorize multiple series.
* @default rainbowSurgePalette
* @deprecated use the `color` prop instead
*/
colors?: ChartContainerProps['colors'];

/**
* Color used to colorize the sparkline.
* @default rainbowSurgePalette[0]
Expand Down Expand Up @@ -149,7 +145,6 @@ const SparkLineChart = React.forwardRef(function SparkLineChart(
height,
margin = SPARKLINE_DEFAULT_MARGIN,
color,
colors: deprecatedColors,
sx,
showTooltip,
showHighlight,
Expand Down Expand Up @@ -214,7 +209,7 @@ const SparkLineChart = React.forwardRef(function SparkLineChart(
...yAxis,
},
]}
colors={colors ?? deprecatedColors}
colors={colors}
sx={sx}
disableAxisListener={
(!showTooltip || slotProps?.tooltip?.trigger !== 'axis') &&
Expand Down Expand Up @@ -265,12 +260,6 @@ SparkLineChart.propTypes = {
* @default rainbowSurgePalette[0]
*/
color: PropTypes.oneOfType([PropTypes.func, PropTypes.string]),
/**
* Color palette used to colorize multiple series.
* @default rainbowSurgePalette
* @deprecated use the `color` prop instead
*/
colors: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.string), PropTypes.func]),
/**
* @default 'linear'
*/
Expand Down
20 changes: 20 additions & 0 deletions packages/x-codemod/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,26 @@
} from '@mui/x-charts-pro/hooks';
```

#### `rename-sparkline-colors-to-color`

Renames the `colors` prop of a `SparkLineChart` to `color` and accesses its first element.

```diff
<SparkLineChart
- colors={['red']}
+ color={['red'][0]}
/>
```

If `colors` is a function, it will be wrapped in another function that returns its first element.

Check warning on line 293 in packages/x-codemod/README.md

View workflow job for this annotation

GitHub Actions / runner / vale

[vale] reported by reviewdog 🐶 [Google.Will] Avoid using 'will'. Raw Output: {"message": "[Google.Will] Avoid using 'will'.", "location": {"path": "packages/x-codemod/README.md", "range": {"start": {"line": 293, "column": 31}}}, "severity": "WARNING"}

```diff
<SparkLineChart
- colors={fn}
+ color={typeof fn === 'function' ? mode => fn(mode)[0] : fn[0]}
/>
```

### Data Grid codemods

#### `preset-safe` for Data Grid v8.0.0
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* eslint-disable no-restricted-imports, @typescript-eslint/no-shadow */
import * as React from 'react';
import { SparkLineChart } from '@mui/x-charts';

const data = [1, 2];

function Chart() {
const fn = (mode) => (mode === 'light' ? ['black'] : ['white']);

// prettier-ignore
return (
<React.Fragment>
<SparkLineChart data={data} colors={['red']} />
<SparkLineChart data={data} colors={fn} />
<SparkLineChart data={data} colors={(mode) => (mode === 'light' ? ['black'] : ['white'])} />
</React.Fragment>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/* eslint-disable no-restricted-imports, @typescript-eslint/no-shadow */
import * as React from 'react';
import { SparkLineChart } from '@mui/x-charts';

const data = [1, 2];

function Chart() {
const fn = (mode) => (mode === 'light' ? ['black'] : ['white']);

// prettier-ignore
return (
(<React.Fragment>
<SparkLineChart data={data} color={['red']?.[0]} />
Copy link
Member Author

Choose a reason for hiding this comment

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

I followed a "better safe than sorry" approach here. Adding the optional chain operator might save us from some crashes in edge cases. Happy to remove it if you think that's too much.

Copy link
Member

Choose a reason for hiding this comment

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

what will happen to var when var=['blue']?

<SparkLineChart data={data} color={typeof fn === "function" ? mode => fn(mode)?.[0] : fn} />
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this is a valid approach.

This introduces a slight runtime performance hit because, when codemodding, we can't guarantee whether fn is a function or an array.

If you think this or another example are too complex, I can just remove this and leave this as an unhandled case.

<SparkLineChart data={data} color={mode => (mode => (mode === 'light' ? ['black'] : ['white']))(mode)?.[0]} />
Copy link
Member Author

Choose a reason for hiding this comment

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

This might seem a bit confusing, but what I'm doing is:

Given an arrow function declaration fnDecl, transform it into: mode => (fnDecl)(mode)?.[0].

We need the optional chaining operator because undefined is a valid value for colors, so we need be careful not to access [0] of an undefined value.

</React.Fragment>)
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { JsCodeShiftAPI, JsCodeShiftFileInfo } from '../../../types';
/**
* @param {import('jscodeshift').FileInfo} file
* @param {import('jscodeshift').API} api
*/
export default function transformer(file: JsCodeShiftFileInfo, api: JsCodeShiftAPI, options: any) {
const j = api.jscodeshift;

const printOptions = options.printOptions;

const root = j(file.source);
const componentNames = ['SparkLineChart'];
const props = { colors: 'color' };

const colorAttributes = root
.find(j.JSXElement)
.filter((path) => {
return componentNames.includes((path.value.openingElement.name as any).name);
})
.find(j.JSXAttribute)
.filter((attribute) => Object.keys(props).includes(attribute.node.name.name as string));

return colorAttributes
.forEach((attribute) => {
if (attribute.node.value?.type !== 'JSXExpressionContainer') {
return;
}

const colorsAttributeExpression = attribute.node.value.expression;

let colorAttributeExpression;

if (colorsAttributeExpression.type === 'ArrayExpression') {
colorAttributeExpression = j.chainExpression(
j.optionalMemberExpression(colorsAttributeExpression, j.literal(0)),
);
} else if (colorsAttributeExpression.type === 'Identifier') {
colorAttributeExpression = j.conditionalExpression(
j.binaryExpression(
'===',
j.unaryExpression('typeof', colorsAttributeExpression),
j.literal('function'),
),
j.arrowFunctionExpression(
[j.identifier('mode')],
j.chainExpression(
j.optionalMemberExpression(
j.callExpression(colorsAttributeExpression, [j.identifier('mode')]),
j.literal(0),
),
),
),
colorsAttributeExpression,
);
} else if (colorsAttributeExpression.type === 'ArrowFunctionExpression') {
colorAttributeExpression = j.arrowFunctionExpression(
[j.identifier('mode')],
j.chainExpression(
j.optionalMemberExpression(
j.callExpression(colorsAttributeExpression, [j.identifier('mode')]),
j.literal(0),
),
),
);
} else {
// Don't know how to handle this case
}

// Only apply transformation if we know how to handle it
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any best practice for letting the user know where we caught an unhandled case? I know we aren't covering 100% of the cases, and some are easy to detect, but hard to fix. It would be nice if we could let the user know which ones have been detected to guide them towards where they need to do a manual fix.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we currently notify unhandled cases

Copy link
Member

Choose a reason for hiding this comment

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

You can propose that though 👍

if (colorAttributeExpression) {
j(attribute).replaceWith(
j.jsxAttribute(
j.jsxIdentifier(props[attribute.node.name.name as string]),
j.jsxExpressionContainer(colorAttributeExpression),
),
);
}
})
.toSource(printOptions);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import path from 'path';
import { expect } from 'chai';
import jscodeshift from 'jscodeshift';
import transform from '.';
import readFile from '../../../util/readFile';

function read(fileName) {
return readFile(path.join(__dirname, fileName));
}

describe('v8.0.0/charts', () => {
describe('rename-sparkline-colors-to-color', () => {
it('transforms code as needed', () => {
const actual = transform(
{
source: read('./actual.spec.tsx'),
path: require.resolve('./actual.spec.tsx'),
},
{ jscodeshift: jscodeshift.withParser('tsx') },
{},
);

const expected = read('./expected.spec.tsx');
expect(actual).to.equal(expected, 'The transformed version should be correct');
});

it('should be idempotent for expression', () => {
const actual = transform(
{
source: read('./expected.spec.tsx'),
path: require.resolve('./expected.spec.tsx'),
},
{ jscodeshift: jscodeshift.withParser('tsx') },
{},
);

const expected = read('./expected.spec.tsx');
expect(actual).to.equal(expected, 'The transformed version should be correct');
});
});
});
Loading