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

[core] Use MUI X official name in errors #11645

Merged
merged 8 commits into from
Jan 10, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jan 10, 2024

Assuming #11556 (comment) logic holds, then this change seems to make the most sense.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Jan 10, 2024
@mui-bot
Copy link

mui-bot commented Jan 10, 2024

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

Generated by 🚫 dangerJS against 348be72

@LukasTy LukasTy changed the title [docs] Use MUI X official name in errors too [docs] Use MUI X official name in errors Jan 10, 2024
@LukasTy LukasTy added core Infrastructure work going on behind the scenes and removed docs Improvements or additions to the documentation labels Jan 10, 2024
@LukasTy LukasTy changed the title [docs] Use MUI X official name in errors [core] Use MUI X official name in errors Jan 10, 2024
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

LGTM. 👌
One note: we decided to use MUI-X-Charts as the error prefix on x-charts code. 🤔
WDYT about that?
Would you rather see MUI X for every package or make the error prefixes package-specific?
I.e.:

  • MUI-X-DataGrid
  • MUI-X-Pickers
  • MUI-X-Charts

P.S. I took the liberty to change the label and PR prefix as it seems to target the core code rather than docs. 🤔

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 10, 2024

we decided to use MUI-X-Charts as the error prefix on x-charts code. 🤔 WDYT about that?

The - look strange to me. Why not "MUI X Charts"?

Otherwise, I think that a broad "MUI X" prefix could be enough, most of the time the error comes with the React component stack trace, so it's not too hard to figure out where the problem is.

@LukasTy
Copy link
Member

LukasTy commented Jan 10, 2024

The - look strange to me. Why not "MUI X Charts"?

Otherwise, I think that a broad "MUI X" prefix could be enough, most of the time the error comes with the React component stack trace, so it's not too hard to figure out where the problem is.

Adding the - could be more familiar to the package name, but I do agree, why not @mui/x-charts then? 🤔
Maybe @alexfauquette can provide more insight into the reason, why he wanted (#11457) the charts error messages to contain the charts specifics?
Or maybe just having them prefixed with MUI X also works for you? 🤔

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 10, 2024
@alexfauquette
Copy link
Member

Or maybe just having them prefixed with MUI X also works for you? 🤔

Yes, My PR was to be sure all charts error have the same prefix. I don't care about which one.

I added charts because errors of this package can be tricky, so if I can make sure users understand which component is complaining, it's good to take

@oliviertassinari oliviertassinari enabled auto-merge (squash) January 10, 2024 18:07
@oliviertassinari oliviertassinari added the dx Related to developers' experience label Jan 10, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 10, 2024

Alright, then a few small steps:

  • renamed "MUI" to "MUI X"
  • renamed "MUI-X-Charts" to "MUI X Charts"
  • adding leading dots where missing
  • adding production bundle size console log removal where missing

In http://github.com/mui/material-ui, I think that it will be good to make the same move, to replace MUI and instead clarify what is MUI System, Material UI or Base UI errors cc @danilo-leal @samuelsycamore. This would be a continuation of #11556 (comment).

@oliviertassinari oliviertassinari merged commit 92b62b9 into mui:next Jan 10, 2024
15 checks passed
@oliviertassinari oliviertassinari deleted the use-official-name branch January 10, 2024 23:27
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 15, 2024

Issue created for the core projects: mui/material-ui#40590 to not forget about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes dx Related to developers' experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants