-
-
Notifications
You must be signed in to change notification settings - Fork 21
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
✨ Chore: Extend Markdoc configuration #127
Conversation
@pietrodev07 |
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 good start! I don't have to many issues that need to be addressed right off the bat... Not sure if @jdtjenkins could help with the react parts.... but shouldn't be to hard to figure out!
case 'react': | ||
return Markdoc.renderers.react(content, React); | ||
case 'react-static': | ||
return Markdoc.renderers.reactStatic(content); |
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.
What is the output of these? In order for them to work with the StudioCMSRenderer.astro
the final output will need to be valid HTML...
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.
react => return ReactNode
react-static => return string so html
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.
for the ReactNode, it might be worth working out a implementation with the Astro ContainerAPI
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.
for the ReactNode, it might be work working out a implementation with the
Astro ContainerAPI
yes, but it’s in an experimental phase, so now we can remove the react option and add it later
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.
This whole project(studioCMS) is in experimental state as well (and we are already using the container api 😛 )
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.
This whole project(studioCMS) is in experimental state as well (and we are already using the container api 😛 )
yeah, so we can use container api...
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.
indeed! Hakuna matata!
I like living on the edge.... we are using ALOT of experimental/beta stuff since astroDB is experimental as well
"astro": ">=4.10.3" | ||
"@types/react": "^18.3.3", | ||
"astro": ">=4.10.3", | ||
"react": "^18.3.1" |
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 don't mind the @types/react
being a standard peerDependeny
but could we add a peerDependenciesMeta
for react
to be optional?
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, make sense, both react and @types/react?
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 just react needs to be moved to an Optional state... but @jdtjenkins could probably give a better answer since i know he had to deal with this when he made the devtoolbar app for AIK
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 just react needs to be moved to an Optional state... but @jdtjenkins could probably give a better answer since i know he had to deal with this when he made the devtoolbar app for AIK
yes, so we wait for his answer
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 for AIK we had react
, @types/react
and react-dom
as optional peer deps if I remember correctly!
{
"peerDependencies": {
"react": "^",
"@types/react": "^",
"react-dom": "^",
},
"peerDependenciesMeta": {
"react": {
"optional": true
},
"@types/react": {
"optional": true
},
"react-dom": {
"optional": true
}
}
}
https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependenciesmeta
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 for AIK we had
react
,@types/react
andreact-dom
as optional peer deps if I remember correctly!{ "peerDependencies": { "react": "^", "@types/react": "^", "react-dom": "^", }, "peerDependenciesMeta": { "react": { "optional": true }, "@types/react": { "optional": true }, "react-dom": { "optional": true } } }https://docs.npmjs.com/cli/v10/configuring-npm/package-json#peerdependenciesmeta
@jdtjenkins yeah, thank you!
@pietrodev07 Getting better! I think Adam's right, I think the Container API would be great for this! Are you able to add a playground project and put together a test page which shows this working too? Then we can easily check that when we make any changes to it, and makes it easier to test it all works! |
yeah, so I’ll work on it, thanks, man! |
Hey @pietrodev07 Any updates? We are getting super close to being ready to go to our first beta, and would love for this to be included to provide full functionality for our MarkDoc option! |
Due to lack of interaction or responses on this PR, the task has been handled in PR #281 and this PR is no longer needed. Thank you @pietrodev07 for the work you did! Please if you get a chance to review #281 before it gets merged, please do and you can be granted co-author credit for it! |
Description