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

feat(theme-classic): MDXContent wrapper component #6842

Merged
merged 3 commits into from
Mar 9, 2022
Merged

Conversation

slorber
Copy link
Collaborator

@slorber slorber commented Mar 4, 2022

Motivation

See #5468 (comment)

That's a legitimate use case to have a global way to wrap/enhance any MDX content being rendered

Also helps reduce a bit boilerplate in page/blog/docs

Have you read the Contributing Guidelines on pull requests?

yes

Test Plan

preview

@slorber slorber added the pr: new feature This PR adds a new API or behavior. label Mar 4, 2022
@slorber slorber requested review from lex111 and Josh-Cena as code owners March 4, 2022 15:00
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Mar 4, 2022
@netlify
Copy link

netlify bot commented Mar 4, 2022

✔️ [V2]

🔨 Explore the source changes: fa5f707

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/62223d8c8713f700077a7f00

😎 Browse the preview: https://deploy-preview-6842--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Mar 4, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 52
🟢 Accessibility 100
🟢 Best practices 92
🟢 SEO 100
🟢 PWA 90

Lighthouse ran on https://deploy-preview-6842--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena changed the title feat: Add theme MDXContent wrapper feat(theme-classic): MDXContent wrapper component Mar 4, 2022
@github-actions
Copy link

github-actions bot commented Mar 4, 2022

Size Change: +70 B (0%)

Total Size: 791 kB

Filename Size Change
website/build/assets/css/styles.********.css 105 kB +70 B (0%)
ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 49.9 kB
website/build/assets/js/main.********.js 597 kB
website/build/index.html 38.4 kB

compressed-size-action

@slorber
Copy link
Collaborator Author

slorber commented Mar 4, 2022

@Josh-Cena let me know if that also helps for your use-case reported here: #5468 (comment)

Copy link
Contributor

@antonk52 antonk52 left a comment

Choose a reason for hiding this comment

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

The latest commit fixes location of MDXContent

The one nit I have is that the tree contains two different components named MDXContent which can be confusing for developers working on themes. As overriding MDXContent in a theme/preset only overrides the top one.

react devtools screenshot

Screenshot 2022-03-04 at 16 40 08

@slorber
Copy link
Collaborator Author

slorber commented Mar 4, 2022

The one nit I have is that the tree contains two different components named MDXContent which can be confusing for developers working on themes. As overriding MDXContent in a theme/preset only overrides the top one.

I also noticed that but I believe it's a devtools bug or something, we only render the component once in the tree.

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Mar 4, 2022

This component doesn't have access to the front matter, so no. But maybe we can provide the current page's metadata in a wrapping context as well?

@Josh-Cena
Copy link
Collaborator

Josh-Cena commented Mar 4, 2022

I also noticed that but I believe it's a devtools bug or something

The second MDXContent looks like it's from the underlying MDX loader return

@slorber
Copy link
Collaborator Author

slorber commented Mar 9, 2022

@Josh-Cena see this issue: #6885

This issue + the proposal above should help simplify your setup, and this change looks like a good thing to do anyway, so I'm going to merge for now.

@slorber slorber merged commit e203001 into main Mar 9, 2022
@slorber slorber deleted the slorber/MDXContent branch March 9, 2022 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants