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] Include history from the @mui/base components #43028

Closed
wants to merge 14 commits into from

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Jul 22, 2024

Check the commits to understand which files are going to persist the history:

  • NoSsr (used initially as a test case) - ccdd8fb
  • all hooks moved from Base UI - 6cc90dc
  • Rest of the components moved from Base UI - ebdc704

I don't think we need to port these changes to master, as soon we are going to replace master with next anyway.

Here is an example file after using git mv last commit vs next.


Considering I used git mv, the history is now moved to the Material UI version of the files, which is okay, as packages/mui-base will eventually be removed.

@mui-bot
Copy link

mui-bot commented Jul 23, 2024

Netlify deploy preview

https://deploy-preview-43028--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against cac3452

@mnajdova mnajdova changed the title [WIP] Include history from the @mui/base components [core] Include history from the @mui/base components Jul 23, 2024
@mnajdova mnajdova marked this pull request as ready for review July 23, 2024 10:54
Copy link
Member

@aarongarciah aarongarciah left a comment

Choose a reason for hiding this comment

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

I haven't checked all files, but the git history looks good in the ones I checked.

Ideally we shouldn't lose git history on Base UI files, but I don't have an opinion since I don't know how much time these files will be around.

@mnajdova
Copy link
Member Author

Ideally we shouldn't lose git history on Base UI files, but I don't have an opinion since I don't know how much time these files will be around.

Let's check after we merge if the history is lost. I am not sure if we can keep the history in both places, considering gh mv is the only option I am aware of for moving the history. Maybe @michaldudak is aware of other option.

@michaldudak
Copy link
Member

According to https://stackoverflow.com/a/44036771 there's some trickery involved if you want to have two copies of a file with history. Have you tried it?

@mnajdova
Copy link
Member Author

According to https://stackoverflow.com/a/44036771 there's some trickery involved if you want to have two copies of a file with history. Have you tried it?

Oh wow, I haven't tried this. But honestly, if we do plan to remove packages/mui-base, I would just keep it as is. If we decide not to do this after all, I will update the PR to do the steps from the link you shared.

I will hold off a bit with merging until we have a clear decision.

@zannager zannager added core Infrastructure work going on behind the scenes package: base-ui Specific to @mui/base labels Jul 23, 2024
@mnajdova mnajdova added package: material-ui Specific to @mui/material and removed package: base-ui Specific to @mui/base labels Jul 24, 2024
@michaldudak
Copy link
Member

If we could do something similar to how docs of old versions of Material UI work - hosting Base UI docs from a branch, we could remove the sources from the repo anytime (unless @mui/base has a future).

@mnajdova
Copy link
Member Author

I am closing in favor of #43076. I've created a script based on the Stackoverflow instructions, it works, we can have history in both locations (I've created new PR to easy up the review without too many commits).

@mnajdova mnajdova closed this Jul 26, 2024
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 package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants