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 re exported utils from packages in docs examples #13901

Closed
wants to merge 6 commits into from

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Jul 19, 2024

This is a follow-up to: #13823 (comment)

  • Add <package>/utils export with utilities, that are used within documentation examples
    • Add paid package re-exporting of such utility from base package (Charts, Pickers)
    • What to do with DataGrid? It already has utils export and it is also re-exported on root. 🙈
  • Use new re-exports in the documentation examples in turn "hiding" the @mui/utils usage from end users

@LukasTy LukasTy added docs Improvements or additions to the documentation core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product labels Jul 19, 2024
@LukasTy LukasTy self-assigned this Jul 19, 2024
@mui-bot
Copy link

mui-bot commented Jul 19, 2024

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

Generated by 🚫 dangerJS against 5079f86

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 31, 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 Jul 31, 2024
@flaviendelangle
Copy link
Member

This PR is several month old, is not a draft but has no review requested
What is its current state?

@zannager zannager requested a review from mnajdova October 17, 2024 12:03
@mnajdova
Copy link
Member

The general direction of the PR makes sense to me, but I wouldn't want to be the reviewer, as I am not fully aware of the scope and usage of these utils in all X products :)

@flaviendelangle
Copy link
Member

The logic seems nice to me
I just wonder what it should look like once we have Base UI X (and stuff like useId is in the Base UI utils package)

Do we export from Base UI X packages AND MUI X packages ?
Do we only export from the Base UI X packages ?
If the Base UI utils package is public, do we say to people to use it directly ?

Just to avoid doing something we would undo in a few months

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 22, 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 Nov 11, 2024
@LukasTy LukasTy removed the request for review from mnajdova November 11, 2024 14:49
@LukasTy
Copy link
Member Author

LukasTy commented Nov 11, 2024

The logic seems nice to me I just wonder what it should look like once we have Base UI X (and stuff like useId is in the Base UI utils package)

Do we export from Base UI X packages AND MUI X packages ? Do we only export from the Base UI X packages ? If the Base UI utils package is public, do we say to people to use it directly ?

Does the relation still matter if we know that the docs are going to be different and the package will be probably standalone as well? 🤔
I think that we would solve this question when such a problem is present. 👌
Personally, I don't see a big problem in showcasing the @mui/utils usage and having it public.
But it has a drawback as it would require the @mui/utils dependency to be present in most cases on user setups. 🙈

@LukasTy LukasTy requested review from a team November 11, 2024 14:57
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 14, 2024
Copy link

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

@michelengelen
Copy link
Member

@LukasTy is this still something we want to pursue?

@LukasTy
Copy link
Member Author

LukasTy commented Dec 27, 2024

@LukasTy is this still something we want to pursue?

That's up to the team and Olivier to decide.
It was the result of exploring a way to "hide" @mui/utils usage. 🤷

@flaviendelangle
Copy link
Member

From what I understand, most (all?) of those utils will move to the base ui utils package.
If this new package is public, then IMHO this PR should be closed
If this new package is internal, then we still have the same topic

@LukasTy
Copy link
Member Author

LukasTy commented Jan 31, 2025

Closing it as it looks like there is no consensus in this regard yet. 🙈
I can return to this PR if we reach a decision. 👌

@LukasTy LukasTy closed this Jan 31, 2025
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 docs Improvements or additions to the documentation PR: out-of-date The pull request has merge conflicts and can't be merged scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants