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

[website] Fix code block on showcase sections #42011

Closed

Conversation

danilo-leal
Copy link
Contributor

@danilo-leal danilo-leal commented Apr 23, 2024

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 (#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

@danilo-leal danilo-leal added bug 🐛 Something doesn't work design This is about UI or UX design, please involve a designer website Pages that are not documentation-related, marketing-focused. labels Apr 23, 2024
@danilo-leal danilo-leal self-assigned this Apr 23, 2024
@mui-bot
Copy link

mui-bot commented Apr 23, 2024

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 🚫 dangerJS against 8ab46aa

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Apr 24, 2024

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.

Screen.Recording.2024-04-24.at.08.09.09.mov

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
Copy link
Member

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
Copy link
Contributor Author

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 #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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

danilo-leal commented Apr 24, 2024

@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.

@danilo-leal danilo-leal requested a review from Janpot April 24, 2024 18:59
@@ -182,15 +180,15 @@ export default function BaseUIComponents() {
>
<HighlightedCode
copyButtonHidden
component={MarkdownElement}
language="jsx"
plainStyles
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
plainStyles
plain

No need for styles as a suffix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uhm, I feel like just "plain" is not very informative, you'd probably need to open up the component to see what it does exactly. We could go with a bit longer name but more descriptive, such as "noBlockStyle" or similar.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

The code higlight here is still a bit off

Screenshot 2024-04-25 at 09 48 48

@Janpot
Copy link
Member

Janpot commented Apr 25, 2024

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 mui/mui-toolpad#3451

Screen.Recording.2024-04-25.at.10.45.57.mov

I also wish the border radius cut clipped the scrollbar.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Apr 25, 2024

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.

@danilo-leal danilo-leal deleted the fix-codeblock-website-sections branch April 25, 2024 13:52
@Janpot
Copy link
Member

Janpot commented Apr 25, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work design This is about UI or UX design, please involve a designer website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants