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

[docs-infra] Add mdx support to vale lint #769

Merged
merged 2 commits into from
Nov 1, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Oct 30, 2024

A quick fix for a regression that I noticed while fixing mui/material-ui#44277. We were no longer linting the docs. This got broken with #538.

Ideally, we could use errata-ai/vale#841, but it's not there. Instead, there are a bunch of examples online on how this can be done https://github.com/search?q=%5Bformats%5D+mdx+%3D+md&type=code.

We will need to propagate this to the other repositories. But unclear if we should do it just yet, we might want to wait.
Point 20. of #588 doesn't make it super clear that MDX is compatible with a great UX for developers. I imagine that it's solvable, but it's it's not it seems to mean that we need to remove MDX and go back to the previous approach that yields better performance.

@oliviertassinari oliviertassinari added the scope: docs-infra Specific to the docs-infra product label Oct 30, 2024
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work regression A bug, but worse labels Oct 30, 2024
@mui-bot
Copy link

mui-bot commented Oct 30, 2024

Netlify deploy preview

https://deploy-preview-769--base-ui.netlify.app/

Generated by 🚫 dangerJS against b18d8d7

Copy link
Contributor

@vladmoroz vladmoroz left a comment

Choose a reason for hiding this comment

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

Point 20. of #588 doesn't make it super clear that MDX is compatible with a great UX for developers. I imagine that it's solvable, but it's it's not, it seems to mean that we need to remove MDX and go back to the previous approach that yields better performance.

I'll look into it, seems that we'd need to lean in more into server components there to fix it instead of passing around the code snippets in React context.

Compressed it's about the same though, comparing the dialog pages:

Screenshot 2024-10-30 at 17 20 27

Screenshot 2024-10-30 at 17 20 30

Neither old or new docs get stellar ratings on PageSpeed Insights too:

Screenshot 2024-10-30 at 17 30 15

Screenshot 2024-10-30 at 17 30 05

We should be able to do better though with just a docs website.

@@ -9,7 +9,13 @@ MinAlertLevel = warning
# 4. You can test locally by replacing the url with the file path of the generated zip
Packages = Google, https://github.com/mui/material-ui/raw/HEAD/docs/mui-vale.zip

[formats]
mdx = md

[*.md]
Copy link
Member Author

Choose a reason for hiding this comment

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

This is kind of magic, it lint mdx too, it doesn't need to be:

Suggested change
[*.md]
[*.{md,mdx}]

so I didn't change it 🤷‍♂️

@michaldudak
Copy link
Member

Point 20. of #588 doesn't make it super clear that MDX is compatible with a great UX for developers.

This isn't caused by MDX, but the Next.js App Router.

@oliviertassinari oliviertassinari merged commit 48c675b into mui:master Nov 1, 2024
18 checks passed
@oliviertassinari oliviertassinari deleted the mdx-vale branch November 1, 2024 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work regression A bug, but worse scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants