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] Warn if axis data don't have enough elements #16830

Merged
merged 3 commits into from
Mar 6, 2025

Conversation

alexfauquette
Copy link
Member

Fix #16740

If we agree on the idea, this could be extended to the line charts

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Mar 5, 2025
@mui-bot
Copy link

mui-bot commented Mar 5, 2025

Deploy preview: https://deploy-preview-16830--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against c229948

@alexfauquette alexfauquette added the bug 🐛 Something doesn't work label Mar 5, 2025
Copy link

codspeed-hq bot commented Mar 5, 2025

CodSpeed Performance Report

Merging #16830 will not alter performance

Comparing alexfauquette:fix-bar-missing-data (c229948) with master (c8a1fdb)

Summary

✅ 7 untouched benchmarks

@@ -41,6 +43,11 @@ export function checkScaleErrors(
`MUI X: ${getAxisMessage(discreteAxisDirection, discreteAxisId)} should have data property.`,
);
}
if (discreteAxisConfig.data.length < series.stackedData.length) {
throw new Error(
`MUI X: ${getAxisMessage(discreteAxisDirection, discreteAxisId)} should have more data than the bar series of id "${seriesId}".`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`MUI X: ${getAxisMessage(discreteAxisDirection, discreteAxisId)} should have more data than the bar series of id "${seriesId}".`,
`MUI X: ${getAxisMessage(discreteAxisDirection, discreteAxisId)} should not have less data than the bar series of id "${seriesId}".`,

have more data than is not correct, as the correct is >=, but putting that into words can become confusing. So we could just invert the phrase and say !<

@bernardobelchior
Copy link
Member

I guess a warning makes sense, but shouldn't we still render the chart? It seems that isn't happening in the docs:

Screen.Recording.2025-03-05.at.13.56.59.mov

Also, shouldn't the fix be to render only the correct number of bars? The warning is useful, but what happens if a user incorrectly spreads some wrong data? Shouldn't we chart still render properly?

@alexfauquette
Copy link
Member Author

Also, shouldn't the fix be to render only the correct number of bars? The warning is useful, but what happens if a user incorrectly spreads some wrong data? Shouldn't we chart still render properly?

I agree for still rendering the charts. By replacing the throw Error() by a console.error(). But I'm not comfortable with the fact of having the chart render properly.

IMO, hiding the extra data points would be hiding the devs error, making bugs harder to spot and fix.

@bernardobelchior
Copy link
Member

I agree for still rendering the charts. By replacing the throw Error() by a console.error(). But I'm not comfortable with the fact of having the chart render properly.

Great!

IMO, hiding the extra data points would be hiding the devs error, making bugs harder to spot and fix.

Yeah, that's a tough trade-off. I think if we show a warning in dev, then it's fine to hide the error by rendering the chart correctly, but it's a matter of preference so not a blocker

@alexfauquette
Copy link
Member Author

I realised line chart altready had similar behavior. Si I replace for line chart the throw Error by a console.error() too. And I updated bar script such that it does not show the error. I hope not too many people will struggle to find their bugs

Copy link
Member

@bernardobelchior bernardobelchior left a comment

Choose a reason for hiding this comment

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

Seems to be fixed, thank you 😄

@alexfauquette alexfauquette merged commit a811f92 into mui:master Mar 6, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Bar showing from the top when x axis data is less than series data
4 participants