Docs/mdx component to markdoc #22030
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Deploy preview for dagster-docs ready! Preview available at https://dagster-docs-9ey9ydf2l-elementl.vercel.app Direct link to changed pages: |
graphite-app
bot
added
the
area: dagster-university
|
Deploy preview for dagster-university ready! ✅ Preview Built with commit 32be57b. |
nikomancy
force-pushed
the
docs/mdx-component-to-markdoc
branch
from
32be57b
to
97ca067
Compare
Basically, making it so buttons don't cause that weird hydration issue with Next.
…ren discovered during testing. Adding unpackText util Using unpackText util in other markdoc tag components.
- Icon does not start at top of the page with text. Markdoc changes the order that CSS classes get applied to child elements compared to preexisting MDX approach so some changes are required to make sure the child elements style correctly. - Child text does not receive colors.text style for some element types. - Child text does not receive text-sm class in some cases. The function applyTextStyles addresses this.
This reverts commit 01203f4. On branch docs/mdx-component-to-markdoc Your branch is up to date with 'origin/docs/mdx-component-to-markdoc'. Changes to be committed: modified: docs/next/markdoc/nodes/index.ts deleted: docs/next/markdoc/nodes/link.markdoc.ts Reverting this commit to have it be a separate pull request for easier reviewing.
cmpadden
force-pushed
the
docs/mdx-component-to-markdoc
branch
from
97ca067
to
537c67e
Compare
cmpadden
approved these changes
May 22, 2024
danielgafni
pushed a commit
to danielgafni/dagster
that referenced
this pull request
Jun 18, 2024
## Summary & Motivation Made in response to issues identified in dagster-io#22025 ### Issue The previous PR caused issues due to both MDX and Markdoc attempting to render using the same Callout component. Markdoc and MDX have different AST formats which results in them passing different children and props to the React components. ### Approach I've added a component for callouts into the MDXComponents.tsx file. This component outputs the same visuals for Callouts including the minor changes we've recently made while restyling the Docs. This duplicate will be able to work with the MDX source and will leave the Markdoc component free to iterate and use the full suite of capabilities Markdoc has without having to worry about interop. While it is tempting to try to have both MDX and Markdoc content render using a single component, there are a set of reasons why this is impractical. - The different ASTs between the two source formats would require logic. - Our MDX implementation requires all components and each of their dependencies be explicitly passed through the MDXComponents.tsx file. Markdoc has cleaner, modularized components and for them to work with MDX, we would need to write the Markdoc components in a way that works with our MDXComponent.tsx approach too. - Given our imminent intent to move away from authoring any further content in MDX, it makes more economic sense to just have duplicate components for our two source content formats in the interim period while we migrate. ## How I Tested These Changes Ran local server. Checked known list of pages that were not working due to previous PR: - http://localhost:3001/getting-started/quickstart - http://localhost:3001/tutorial/introduction - http://localhost:3001/tutorial/introduction - http://localhost:3001/tutorial/setup - http://localhost:3001/tutorial/building-an-asset-graph - http://localhost:3001/tutorial/scheduling-your-pipeline - http://localhost:3001/tutorial/connecting-to-external-services Confirmed Callouts look the same between an MDX page (http://localhost:3001/getting-started/quickstart) and a Markdoc page (http://localhost:3001/getting-started/install)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Made in response to issues identified in #22025
Issue
The previous PR caused issues due to both MDX and Markdoc attempting to render using the same Callout component. Markdoc and MDX have different AST formats which results in them passing different children and props to the React components.
Approach
I've added a component for callouts into the MDXComponents.tsx file. This component outputs the same visuals for Callouts including the minor changes we've recently made while restyling the Docs. This duplicate will be able to work with the MDX source and will leave the Markdoc component free to iterate and use the full suite of capabilities Markdoc has without having to worry about interop.
While it is tempting to try to have both MDX and Markdoc content render using a single component, there are a set of reasons why this is impractical.
How I Tested These Changes
Ran local server. Checked known list of pages that were not working due to previous PR:
Confirmed Callouts look the same between an MDX page (http://localhost:3001/getting-started/quickstart)
and a Markdoc page (http://localhost:3001/getting-started/install)