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

[charts] Fix eslint for react compiler #13444

Merged
merged 2 commits into from
Jun 13, 2024
Merged

[charts] Fix eslint for react compiler #13444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df81089
[charts] Fix eslint for react compiler
alexfauquette Jun 11, 2024
117d0b9
prettier
alexfauquette Jun 11, 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
3 changes: 1 addition & 2 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@ const path = require('path');
const ENABLE_REACT_COMPILER_PLUGIN = process.env.ENABLE_REACT_COMPILER_PLUGIN ?? false;

// Enable React Compiler Plugin rules per package
const ENABLE_REACT_COMPILER_PLUGIN_CHARTS =
process.env.ENABLE_REACT_COMPILER_PLUGIN_CHARTS ?? false;
const ENABLE_REACT_COMPILER_PLUGIN_CHARTS = process.env.ENABLE_REACT_COMPILER_PLUGIN_CHARTS ?? true;
const ENABLE_REACT_COMPILER_PLUGIN_DATA_GRID =
process.env.ENABLE_REACT_COMPILER_PLUGIN_DATA_GRID ?? false;
const ENABLE_REACT_COMPILER_PLUGIN_DATE_PICKERS =
Expand Down
14 changes: 7 additions & 7 deletions packages/x-charts/src/ChartsReferenceLine/ChartsXReferenceLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
ChartsReferenceLineClasses,
getReferenceLineUtilityClass,
} from './chartsReferenceLineClasses';
import { buildWarning } from '../internals/warning';

export type ChartsXReferenceLineProps<
TValue extends string | number | Date = string | number | Date,
Expand Down Expand Up @@ -72,7 +73,11 @@ export function getXReferenceLineClasses(classes?: Partial<ChartsReferenceLineCl
);
}

let warnedOnce = false;
const valueError = buildWarning(
(value, id) =>
`MUI X Charts: the value ${value} does not exist in the data of x axis with id ${id}.`,
'error',
);

function ChartsXReferenceLine(props: ChartsXReferenceLineProps) {
const {
Expand All @@ -93,12 +98,7 @@ function ChartsXReferenceLine(props: ChartsXReferenceLineProps) {

if (xPosition === undefined) {
if (process.env.NODE_ENV !== 'production') {
if (!warnedOnce) {
warnedOnce = true;
console.error(
`MUI X Charts: the value ${x} does not exist in the data of x axis with id ${axisId}.`,
);
}
valueError(x, axisId);
}
return null;
}
Expand Down
16 changes: 8 additions & 8 deletions packages/x-charts/src/ChartsReferenceLine/ChartsYReferenceLine.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
ChartsReferenceLineClasses,
getReferenceLineUtilityClass,
} from './chartsReferenceLineClasses';
import { buildWarning } from '../internals/warning';

export type ChartsYReferenceLineProps<
TValue extends string | number | Date = string | number | Date,
Expand Down Expand Up @@ -60,8 +61,6 @@ const getTextParams = ({
}
};

let warnedOnce = false;

export function getYReferenceLineClasses(classes?: Partial<ChartsReferenceLineClasses>) {
return composeClasses(
{
Expand All @@ -74,6 +73,12 @@ export function getYReferenceLineClasses(classes?: Partial<ChartsReferenceLineCl
);
}

const valueError = buildWarning(
(value, id) =>
`MUI X Charts: the value ${value} does not exist in the data of y axis with id ${id}.`,
'error',
);

function ChartsYReferenceLine(props: ChartsYReferenceLineProps) {
const {
y,
Expand All @@ -93,12 +98,7 @@ function ChartsYReferenceLine(props: ChartsYReferenceLineProps) {

if (yPosition === undefined) {
if (process.env.NODE_ENV !== 'production') {
if (!warnedOnce) {
warnedOnce = true;
console.error(
`MUI X Charts: the value ${y} does not exist in the data of y axis with id ${axisId}.`,
);
}
valueError(y, axisId);
}
return null;
}
Expand Down
4 changes: 1 addition & 3 deletions packages/x-charts/src/ChartsText/ChartsText.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,11 @@ function ChartsText(props: ChartsTextProps) {
if (angle) {
transforms.push(`rotate(${angle}, ${x}, ${y})`);
}
if (transforms.length) {
textProps.transform = transforms.join(' ');
}

return (
<text
{...textProps}
transform={transforms.length > 0 ? transforms.join(' ') : undefined}
x={x}
y={y}
textAnchor={textAnchor}
Expand Down
17 changes: 17 additions & 0 deletions packages/x-charts/src/internals/warning.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
export function buildWarning(
Copy link
Member

Choose a reason for hiding this comment

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

I would probably just rename it to buildOneTimeWarning or buildWarningOnce

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a common piece of code we duplicate in each package.

Was introduced mostly to avoid code duplication in the data grid. We could do the renaming but in that case, would require to do it in all packages

Copy link
Member

Choose a reason for hiding this comment

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

up to you, doesn't matter much 😅

should we eventually have a shared X package? shouldn't be too hard to accomplish

Copy link
Member

Choose a reason for hiding this comment

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

should we eventually have a shared X package?

This has been discussed quite recently
I feel like we are only waiting for a use-case and someone to own the creation of the new package 😆

@joserodolfofreitas @cherniavskii if you are OK, we can create it

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, we need a shared utils package 👍🏻

message: (...args: any) => string,
gravity: 'warning' | 'error' = 'warning',
) {
let alreadyWarned = false;

return (...args: any) => {
if (!alreadyWarned) {
alreadyWarned = true;
if (gravity === 'error') {
console.error(message(...args));
} else {
console.warn(message(...args));
}
}
};
}