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

[wesbite] Remove duplicate MarkdownElement component #42028

Merged
merged 3 commits into from
Apr 29, 2024

Conversation

danilo-leal
Copy link
Contributor

This PR is a continuation of #42022.

  • removed the duplicate MarkdownElement component, whose sole purpose was to strip away the "block" design from the code block so it worked properly within the marketing pages showcase section
  • regardless, we still need something to toggle the styles on and off, so I added a plainStyle prop to the HighlightedCode component, which now controls how the pre element is rendered. That concentrates these different design options in a single component, thus not having two MarkdownElement components with essentially the same purpose

This has been explored in #42011 and it seemed to make sense there!

@danilo-leal danilo-leal added the website Pages that are not documentation-related, marketing-focused. label Apr 26, 2024
@danilo-leal danilo-leal self-assigned this Apr 26, 2024
@mui-bot
Copy link

mui-bot commented Apr 26, 2024

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 2e71ae3

Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks good, same comment as in #42011 (comment)

The highlight seems to deal badly with horizontal overflow:

Screenshot 2024-04-29 at 11 32 29

And this is probably not in scope for this PR, but I noticed some layout shifts on the homepage when switching between examples, once to go to the loading state, and a second time when rendering the example:

Screen.Recording.2024-04-29.at.11.34.21.mov

@danilo-leal
Copy link
Contributor Author

I can maybe tackle these on a separate PR? There's another PR #38604 tackling the homepage product suite section that might tackle the layout shift a bit. And the code highlight there might be trickier to tackle, I imagine it'll require wrapping the code lines using Prism, somehow.

@Janpot
Copy link
Member

Janpot commented Apr 29, 2024

And the code highlight there might be trickier to tackle, I imagine it'll require wrapping the code lines using Prism, somehow.

Perhaps we can use a plugin for that?

@danilo-leal
Copy link
Contributor Author

That sounds like a good idea! And it'd tackle a docs-infra issue (#41509) I had opened a while ago, too :)

@danilo-leal
Copy link
Contributor Author

However, I assume the bad thing about going with the "native Prism" option is losing the animation. Either way, let me know if this PR is in good shape to be merged, and then we can continue discussing these follow-up improvements elsewhere! 😬

@danilo-leal danilo-leal requested a review from Janpot April 29, 2024 11:10
Copy link
Member

@Janpot Janpot left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@danilo-leal danilo-leal merged commit 5651fdd into mui:next Apr 29, 2024
23 checks passed
@danilo-leal danilo-leal deleted the remove-duplicate-markdownelement branch April 29, 2024 11:49
@Janpot
Copy link
Member

Janpot commented Apr 29, 2024

However, I assume the bad thing about going with the "native Prism" option is losing the animation.

Personally, I wouldn't mind. The way the plugin does highlighting is more familiar to how code editors highlight, and the smooth scroll when scrolling it into view would be enough visual feedback for me. In any case, even if we want to keep the animation, building this as a prisma plugin could give us access to the internals we need for a stable implementation.

export interface HighlightedCodeProps {
code: string;
component?: React.ElementType;
Copy link
Member

Choose a reason for hiding this comment

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

I can see that the no-breaking-changes-in-shared-packages principle will be pretty hard to enforce...
@Janpot, @mnajdova

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants