Skip to content

[website] Fix /base-ui/ code duplication#416

Merged
oliviertassinari merged 5 commits intomui:masterfrom
oliviertassinari:fix-invalid-styled-call
Jun 2, 2024
Merged

[website] Fix /base-ui/ code duplication#416
oliviertassinari merged 5 commits intomui:masterfrom
oliviertassinari:fix-invalid-styled-call

Conversation

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 26, 2024

Randomly delete files of https://mui.com/base-ui/ that are duplicated to make sure we don't try to use them (they will go out of sync).

Preview: https://deploy-preview-416--base-ui.netlify.app/

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation. website labels May 26, 2024
@mui-bot
Copy link

mui-bot commented May 26, 2024

Netlify deploy preview

https://deploy-preview-416--base-ui.netlify.app/

Generated by 🚫 dangerJS against 81bfc4e

@michaldudak
Copy link
Member

I don't understand the purpose of this PR. These files are indeed duplicated, but this copy should be the source of truth as we're going to host the live docs from this repo.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 28, 2024

I removed the source of this page https://mui.com/base-ui/ and only because the source we have in Base UI isn't used in production, we made changes to the source of https://mui.com/base-ui/ since from http://github.com/mui/material-ui. I believe it's clearer this way (no duplication).

@oliviertassinari oliviertassinari force-pushed the fix-invalid-styled-call branch from 86f5eee to 4837f72 Compare May 28, 2024 18:04
@michaldudak
Copy link
Member

It doesn't really matter if they aren't in sync. What we have here will ultimately replace the landing page that's currently live at mui.com.
The changes proposed in this PR make PR previews basically unusable if you don't have the deep URL: https://deploy-preview-416--base-ui.netlify.app/base-ui/

cc @colmtuite

@colmtuite
Copy link
Contributor

@michaldudak Agreed, it's fine if they're out-of-sync.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 29, 2024

Would it work if we make it behave like with MUI X https://master--material-ui-x.netlify.app (the redirection) in the meantime? No strong preference, my initial objective was to make it impossible to not sync back these pages in Base UI from their current state in the main repository.

What we have here will ultimately replace the landing page that's currently live at mui.com.

Oh, interesting, so the plan is Option C? https://www.notion.so/mui-org/docs-infra-Marketing-site-code-location-70fa836aeb654e2eb429721706529e1d?pvs=4#a0a3f8db560b42ce8d6c4626b6741fc8. It wasn't clear to me if we found a solution to this problem.

Off-topic. On what we have in the page today, some of the ideas to push it further mui/material-ui#36622 (comment)

@colmtuite
Copy link
Contributor

@oliviertassinari My preference is option B, but it seems option C won.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label May 30, 2024
@oliviertassinari oliviertassinari force-pushed the fix-invalid-styled-call branch from d41d641 to 1937c33 Compare June 2, 2024 18:05
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jun 2, 2024
@oliviertassinari
Copy link
Member Author

Ok, for now I'm aiming for a simple goal with this PR: removing the code duplication in the codebase. I have fixed the problem @michaldudak raised with the URLs by coping MUI X's approach. We now have these links that I believe behave correctly:

Going forward, the fate of https://github.com/mui/material-ui/blob/next/docs/pages/base-ui.tsx will depend on https://www.notion.so/mui-org/docs-infra-Marketing-site-code-location-70fa836aeb654e2eb429721706529e1d.

@oliviertassinari oliviertassinari changed the title [docs] Remove dead files [core] Fix /base-ui/ code duplication Jun 2, 2024
@oliviertassinari oliviertassinari added core and removed docs Improvements or additions to the documentation. labels Jun 2, 2024
@oliviertassinari oliviertassinari changed the title [core] Fix /base-ui/ code duplication [webste] Fix /base-ui/ code duplication Jun 2, 2024
@oliviertassinari oliviertassinari changed the title [webste] Fix /base-ui/ code duplication [website] Fix /base-ui/ code duplication Jun 2, 2024
@oliviertassinari oliviertassinari merged commit ebccdf3 into mui:master Jun 2, 2024
@oliviertassinari oliviertassinari deleted the fix-invalid-styled-call branch June 2, 2024 19:35
@michaldudak
Copy link
Member

Oh, interesting, so the plan is Option C? https://www.notion.so/mui-org/docs-infra-Marketing-site-code-location-70fa836aeb654e2eb429721706529e1d?pvs=4#a0a3f8db560b42ce8d6c4626b6741fc8. It wasn't clear to me if we found a solution to this problem.

I wasn't aware of this discussion. It felt natural to me to keep all Base UI-related docs in our repo so we can own them, style them and host them separately if there's a need to.
It feels weird to me that everything but the landing page is in our repo, but if y'all prefer it this way, so be it.

@colmtuite
Copy link
Contributor

@oliviertassinari @michaldudak Can we undo this merge somehow? We previously decided to keep the marketing page and docs separate, because we're not sure yet how to manage the styling/structure/usability/a11y of the docs and website. Also, nobody approved this PR, and I don't think any PRs should be merged without approval.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 3, 2024

@colmtuite We can definitely do the opposite (remove the page from Material UI repository and have Base UI page what's shows in production from this repository). The problem is that we fixed stuff in https://github.com/mui/material-ui/blob/next/docs/pages/base-ui.tsx that isn't propagated in this repository, the content we have here is outdated. So IMHO, it's not about reverting this PR, but transfering the code from the Material UI repository to here and then putting it in production.

@michaldudak
Copy link
Member

My take on it is that we should revert this PR but keep the landing page in the Core repo for now. Once we're ready with the new website (have enough components to build a nice landing page, etc.), we remove all Base UI-related code and docs from the Core repo and either have everything in this repo (option C) or in yet another repo (option C).

@colmtuite
Copy link
Contributor

Yes I agree with @michaldudak.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 4, 2024

It feels like I'm missing something, I would have expected this PR to go without friction (why I merged straight, I wanted a PR to see the CI checks, why I didn't push a commit straight to HEAD).
What problem are we trying to solve with having https://github.com/mui/material-ui/blob/next/docs/pages/base-ui.tsx in this repository, but with outdated content (code that we will have to delete eventually since would have forked too much) and that is only visible in PR previews? I fail there 😄. I'm happy with any path that doesn't have code duplicating in the codebase (our codebase is the HEAD of mui-private + mui-public + material-ui + base-ui + pigment-css repositories).

@michaldudak michaldudak added scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). and removed core labels May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants