mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.73k stars 31.51k forks source link

[website] Fix code block on showcase sections #42011

Closed danilo-leal closed 1 week ago

danilo-leal commented 1 week ago

I'm not fully sure yet what caused this, but all of the code block components within these showcase sections got broken. I wonder if it has anything to do with the HighlightedCode component migration to the @mui/docs package (https://github.com/mui/material-ui/pull/41859), but I couldn't find anything outstanding there, given it was just moving the files to a different location...


Update: Prior to the PR linked above, the marketing showcase code blocks were pulling their styles from a custom MarkdownElement component different from the one we use in the docs. And, given that PR then changed all of the MarkdownElement-related imports to the same one, the styles broke.

So, this PR adds custom pre element styling to the HighlightedCode component via a new plainStyles prop that allows us to use a stripped-down styled version of the code block that is primarily useful, at least for now, in the marketing showcase sections.

Before After
Screenshot 2024-04-23 at 20 10 47 Screenshot 2024-04-23 at 20 10 52
Screenshot 2024-04-23 at 20 11 06 Screenshot 2024-04-23 at 20 11 12
Screenshot 2024-04-23 at 20 11 42 Screenshot 2024-04-23 at 20 11 49
mui-bot commented 1 week ago

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad) No bundle size changes

Generated by :no_entry_sign: dangerJS against 8ab46aa2e18c02a58e01786f1130a301191d35b9

danilo-leal commented 1 week ago

This is so weird — I don't see any of the changes I applied working on the deploy preview, and some of the fixes that were working previously, now that I've rerun the PR, are not anymore. @mnajdova or @siriwatknp any pointers here? 🤔

For example, the homepage code block is fixed on the local host but not in the deploy preview.

Deploy preview Local host
Screenshot 2024-04-24 at 08 03 03 Screenshot 2024-04-24 at 08 03 06

Another example is in this Material Components section; yesterday, I had it working with overflow: 'clip', but not anymore. However, if I switch it to hidden, the styles defined in the Frame.Info component are applied 😵‍💫 I have no idea why this would happen if it was working before.

https://github.com/mui/material-ui/assets/67129314/0c5f2a22-ec70-4785-9276-0b6af2048d0d

And to add to that, the above-displayed section on the video is working perfectly fine in the deploy preview 😅 It's just when I run it locally that the styles break.

mnajdova commented 1 week ago

this PR fixes that, plus adds a few other improvements.

Can we scope the PRs to do one thing? Especially if they are regressions. We should also link which PR caused the regression

danilo-leal commented 1 week ago

Can we scope the PRs to do one thing? Especially if they are regressions. We should also link which PR caused the regression

Sure, happy to narrow it down! Thing is, I'm not fully sure which PR or change is the exact culprit for what I'm trying to solve here. As I mentioned in the description, the most obvious candidate would be https://github.com/mui/material-ui/pull/41859, as it was the latest PR that tackled the related files, but I'm not seeing any functional changes aside from moving the components to a new location...

mnajdova commented 1 week ago

You are right about the PR in question. We had two different versions of the MarkdownElement component, one in docs/src/modules/components/MarkdownElement and one in docs/src/components/markdown/MarkdownElement, and they were both replaced by the same version. I think we just need to fix the imports. Let me know if this helps.

mnajdova commented 1 week ago

I would advise scoping the changes to only fix the regression, I know it's small adjustments, but if we have another regression it would be hard to track it back.

danilo-leal commented 1 week ago

@mnajdova I could remove some tiny little tangential style adjustments, but they were all ultimately fixing broken stuff on these showcase-related sections, so they seem to make sense? Ultimately, this PR is mostly about fixing everything related to the code blocks on them.

Janpot commented 1 week ago

I wasn't careful enough when renaming the imports. I'm also in favor of correcting the regression first and then improve the implementation. It does seem to be fixed in production, right? Or did we roll back docs deploy? As it was caused by me, is there anything I can do to help?

🙂 Have also a minor comment about the implementation. It feels strange to me to see the padding on the scroll container and not on the content. May be preference thing though, but correcting what was scroll container vs content was most of the fix in https://github.com/mui/mui-toolpad/pull/3451

https://github.com/mui/material-ui/assets/2109932/0ad1d54a-8030-4cce-b177-afb7ff7e4946

I also wish the border radius cut clipped the scrollbar.

danilo-leal commented 1 week ago

I would advise scoping the changes to only fix the regression I'm also in favor of correcting the regression first and then improve the implementation.

@mnajdova + @Janpot just so we're all on the same page before moving any longer forward — if I were to just fix the bug, I'd only change the import from @mui/docs/MarkdownElement to docs/src/components/markdown/MarkdownElement on the showcase-focused components within the marketing pages scope. That would solve the designs that got messed up.

So, is it preferable to have a PR focused on only that first and then carry this one on with enhancements on top of the fix, or would it be cool to carry on here? I can do both, but if we go with the first option, just for the sake of cleanliness—as this PR has a lot of convo going on already— I'd open a new one with the simple fix and then continue onwards here (take the fix to a better implementation + tackle smaller additional details).


Update: okay, immediately after writing the above, I got convinced that the simple fix would be simpler. 😅 I'll close this PR and then open two new ones in this order:

  1. One with just the simple import fix
  2. A second one, which would be virtually the same as this one, but starting with a blank slate, where I remove the duplicate MarkdownElement component and optimize this + other small details. We can continue the convo in there.
Janpot commented 1 week ago

Yes, absolutely, two PRs allow us to progress at different speeds 👍