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

Refactor to improve wording #2513

Merged
merged 3 commits into from
Jul 16, 2024
Merged

Refactor to improve wording #2513

merged 3 commits into from
Jul 16, 2024

Conversation

filippovd20
Copy link
Contributor

@filippovd20 filippovd20 commented Jul 14, 2024

importing mdx not md

Initial checklist

  • I read the support docs
  • I read the contributing guide
  • I agree to follow the code of conduct
  • I searched issues and couldn’t find anything (or linked relevant results below)
  • If applicable, I’ve added docs and tests

Description of changes

Fixing a typo in the code snipped importing from mdx not md directly.

importing mdx not md

Signed-off-by: Dmitry Filippov <[email protected]>
Copy link

vercel bot commented Jul 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 16, 2024 9:24am

@codecov-commenter
Copy link

codecov-commenter commented Jul 14, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (908ff45) to head (e5d01a0).
Report is 40 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #2513   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines         2693      2712   +19     
  Branches         2         2           
=========================================
+ Hits          2693      2712   +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wooorm
Copy link
Member

wooorm commented Jul 14, 2024

Hi! Why do you think this is a mistake? I think this is intended!

@remcohaszing
Copy link
Member

I think the comment should say Assumes an integration is used to compile markdown -> JS., not Assumes an integration is used to compile MDX -> JS..

@wooorm
Copy link
Member

wooorm commented Jul 14, 2024

I kinda get what you're saying but saying "md" there feels a bit more static. Like using marked/remark/markdown-it.
Perhaps the "to JS" part cancels that out enough.
The point, I think, when I added those comments, is that it doesn't work normally, to import these files. They need some integration, not one particular one. Likely one of ours, but not always.

@filippovd20
Copy link
Contributor Author

@wooorm @remcohaszing if you look into the code snippet from post.mdx, both License and Contributing are regular components with props. Contributing is imported from an mdx module assuming everything is configured as expected. License on the other hand is imported from .md which is definitely a typo. There's no way you can import a component from an md file directly. If you could do that then why bother about mdx then?

@wooorm
Copy link
Member

wooorm commented Jul 15, 2024

There's no way you can import a component from an md file directly. If you could do that then why bother about mdx then?

Yes you can, our packages can do that! Please see what MDX is, particularly this note: https://mdxjs.com/docs/what-is-mdx/#mdx-syntax.
Well, one reason where you go wrong: markdown doesn’t have imports so markdown couldn’t import markdown.

@filippovd20 filippovd20 marked this pull request as draft July 15, 2024 18:51
fixing additional examples in the docs to import mdx not md

Signed-off-by: Dmitry Filippov <[email protected]>
@filippovd20
Copy link
Contributor Author

@wooorm I'm wrong and you're right. Indeed with the bundler configured it also compiles regular markdown to JS which is also described in Quick Start. Do you think changing the comment would be beneficial to stress the fact that md is also compiled to JS? something like this:
// Assumes a bundler is used to compile MD -> JS

docs/docs/using-mdx.mdx Outdated Show resolved Hide resolved
docs/docs/using-mdx.mdx Outdated Show resolved Hide resolved
docs/docs/using-mdx.mdx Outdated Show resolved Hide resolved
@wooorm wooorm marked this pull request as ready for review July 16, 2024 09:24
@wooorm wooorm changed the title fix:using-mdx.mdx fixing typo Refactor to improve wording Jul 16, 2024
@wooorm wooorm merged commit 07d5e2f into mdx-js:main Jul 16, 2024
6 checks passed
@wooorm wooorm added 📚 area/docs This affects documentation 💪 phase/solved Post is done labels Jul 16, 2024
@wooorm
Copy link
Member

wooorm commented Jul 16, 2024

thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📚 area/docs This affects documentation 💪 phase/solved Post is done
Development

Successfully merging this pull request may close these issues.

4 participants