-
Notifications
You must be signed in to change notification settings - Fork 11
feat(mdx): Add script for React mdx conversion #67
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
Conversation
| "react": "^18.3.1", | ||
| "react-docgen": "^7.1.1", | ||
| "react-dom": "^18.3.1", | ||
| "react-error-boundary": "^6.0.0", |
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.
Using this package as a quick and easy error boundary to wrap the examples in so that errors encountered when trying to render them don't crash the UI while I fine tuned the conversion. It has a few million weekly downloads on NPM so it's probably fine, but I have no strong feeling about keeping it in for the real PR.
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've used it and liked it in the past fwiw.
| import * as reactIconsModule from '@patternfly/react-icons' | ||
| import styles from "@patternfly/react-styles/css/components/_index" | ||
| import * as reactTokensModule from "@patternfly/react-tokens" |
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 think we'll want to refactor this so that we pass all of these modules via the config, but for now just doing this works. What do yall think?
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.
That makes sense 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.
Would you be opposed to that being done in a followup?
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.
Made this issue to address #72
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 think this is fine for now.
| import PropsTables from '../../components/PropsTables.astro' | ||
| import CSSTable from '../../components/CSSTable.astro' | ||
| import SectionGallery from '../../components/section-gallery/SectionGallery.astro' | ||
| import LiveExample from '../../components/LiveExample.astro' |
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.
Changes to this file were needed because by default the examples are rendering at this endpoint rather than /[...tab], though I'm not sure why off hand.
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 think its because the nav links to components/alert (hitting page.astro and then rewriting to tab.astro) and not components/alert/react? Though it should be ultimately only hitting tab.astro though so I'm not sure either.
dlabaj
left a comment
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.
A few comments.
convertToMDX.ts
Outdated
| @@ -0,0 +1,34 @@ | |||
| #!/usr/bin/env ts-node | |||
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.
Do we need this if we are using tsx instead of ts-node?
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.
Ah nope I don't think so, good catch
convertToMDX.ts
Outdated
| import { glob } from 'glob' | ||
|
|
||
| async function main() { | ||
| const files = await glob('./testContent/react/examples/*.md') |
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.
Can we add a way to have this be passed in via the command line.
convertToMDX.ts
Outdated
| import { readFile, writeFile } from 'fs/promises' | ||
| import { glob } from 'glob' | ||
|
|
||
| async function main() { |
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'm wondering if this should be part of the cli.
| import * as reactIconsModule from '@patternfly/react-icons' | ||
| import styles from "@patternfly/react-styles/css/components/_index" | ||
| import * as reactTokensModule from "@patternfly/react-tokens" |
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.
That makes sense to me.
|
🎉 This PR is included in version 1.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Closes #64
convertToMdx.tsis the script which does the conversion, you can run it withnpx tsx cli/cli.ts convert-to-mdx <glob path>after copying an example directory in if you'd like to test executing it yourself.