-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Document passing components to MDX Content #12597
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do already document this here: https://docs.astro.build/en/guides/integrations-guide/mdx/#custom-components-with-imported-mdx
Maybe we can link there? Or tweak it if we need to clarify anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @matthewp , this is a helpful example! See my comments below, including a question making sure it's clear how including the default exports work!
Following up, now Chris's comment is appearing for me. I'm going to suggest we keep this content in the MDX page only and perhaps link to it from the content collections page. Maybe it's the MDX page that needs beefing up so it's obvious that this works in content collections, too! |
OK! So I updated both the text on the content collections and the MDX pages so that the main content is all on the MDX page and content collections links to it. I made this intentionally as big as it could be, showing both a plain "import from MDX" and a content collections example. Would love help fixing up the MDX stuff to make sure it's helpful enough for both scenarios, but not too much! @delucis |
(Noting two link issues we can address once we've decided on a section heading) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to me! I left a comment for the heading and another nit because I think "blog posts" might confuse some people?
<ReactCounter client:load /> | ||
``` | ||
|
||
#### Custom components with imported MDX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Custom components with MDX" instead. I think "imported" might confuse people because when they use content collections, they are not importing the files themselves.
Otherwise, to fix the link directly, "Passing components to MDX content" sounds fine to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think this is the perfect heading as-is yet, so def want us to workshop this (especially as we know it will cause link chaos when we change it!)
I worry that "Custom components with MDX" is a little too generic? Like maybe this could be importing your UI components into MDX?
"Passing components to MDX content" was the best thought I had so far, and nothing else on this page is very API-forward e.g. "Using the components
prop" so it probably should stay very use-case based.
Even still, the "passing" emphasizes the implementation, not the end goal. It's really more like "Assigning custom elements to HTML elements... when using the <Content />
component to pull in and render your Markdown. Maybe this should in fact be a sub-section of THAT section?
UPDATE: reorganized (but did not change section heading as it's not quite as bad in this flow now?) Can still debate whether it needs changing!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, I understand what you mean. I agree "Custom components with MDX" might be confusing!
I haven't had a chance to see the live preview yet 😅 , but looking at the "code" changes, the reorganization looks better. I'm still a bit concern regarding this heading. We precise this include content collections but maybe someone scanning the page would miss that seeing "imported"?
What I can think of "Overwrite HTML elements with custom components" or "Customizing HTML elements with MDX" but... this sounds a bit duplicated with the previous section. Maybe "Using custom components from your Astro components"? I can't find something better, so maybe what we have currently is fine!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like "Passing components to MDX content" of the options discussed, but will leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, now the note has moved I think "Passing" makes sense here. I don't know if that's the right word (I mean balanced, "passing" seems fine!), but it seems more balanced to me: Assigning
/Passing
. And because this reused the same wording as the note it shouldn't confuse users.
Co-authored-by: Armand Philippot <[email protected]>
OK, I addressed Yan's grammar nits, and @ArmandPhilippot , what do you think of this new order/flow? I honestly think this whole thing feels much better in this order! |
Updating branch to hopefully get a deploy preview |
Description (required)
Documents passing components to MDX
<Content />
component.Related issues & labels (optional)
For withastro/astro#14591