-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[code-infra] Move the HighlightedCode component to @mui/docs #41859
Conversation
Netlify deploy previewhttps://deploy-preview-41859--material-ui.netlify.app/ Bundle size report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. The X migration should not be an issue: mui/mui-x#12757
@bharatkashyap Just a heads up since this PR could interfere with your #40901 by moving the MarkdownElement
Yes, I noticed that. I can resolve conflicts once this is merged |
I added a commit to add diff from #39603 the PR I merged before reviewing this one |
@Janpot there are two MarkdownElement component in the repository, some wrong imports were changed (@danilo-leal is fixing this in #42011, so we are good on core side). Have you checked which imports were the X and Toolpad repo using before? Just making sure we didn't move the wrong component. I advised @danilo-leal to rename the other component, but it would be great to check this before the change propagates everywhere. The components in question are: |
On the X side, we have |
Heads-up on that: on my PR that Marija linked above, I'm changing this component's name to |
Yes, as soon as the core do a release, I will update the X repo to get the latest |
I'm removing the one that I found in Toolpad. |
Uhm, yeah. There might be a better way to structure the |
I think the main thing that I found not ergonomic is the height + overflow + border radius. It also feels like the wrong element is the scroll container
Corners fighting with scrollbars 🙂 |
I propose we go step by step:
|
That makes sense! In the meantime, I recorded this quick Loom just to walk through what I was thinking as a solution to remove the We don't need to tackle it immediately, but if this makes sense, I'm happy to do it! It's also not tackling the stuff you mentioned @Janpot just yet. It's just a way to consolidate custom |
@danilo-leal that makes sense, I would suggest renaming the prop to |
For the record, I'm just pointing out some rough edges I ran into when integrating |
https://www.notion.so/mui-org/base-ui-Stable-release-outline-3f2197f1885747bdb3f384b30697a337
Moving components to @mui/docs: