-
Notifications
You must be signed in to change notification settings - Fork 4
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
Adopt svelte-kit package and add Markdoc.svelte #3
base: main
Are you sure you want to change the base?
Conversation
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.
Nice work!!
Left a couple comments/thoughts, but I like where it's headed.
For types, we could switch everything to TS or just use jsdoc.. maybe for now jsdoc is enough? That's what I did on the svelte-stripe project and it seemed to work well.
For testing, we can add playwright support, probably can be done in a separate PR though.
LMK, I can set it up.
@@ -20,11 +20,4 @@ function render(source, config) { | |||
const configWithFrontmatter = addFrontmatter(ast, config) | |||
const content = markdoc.transform(ast, configWithFrontmatter) | |||
return markdoc.renderers.html(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.
Maybe instead of using Markdoc's renderer, it can reuse Markdoc.svelte
here?
That way tags can work for the preprocessor approach too.
The preprocessor can use the config to figure out what tags are supported.
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 great idea! I've not looked into the preprocessor API before but it looks like this kind of thing should be possible?
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.
Yes, all the preprocessor needs to do is return a string containing valid svelte code. I think it should be doable.
src/lib/renderer/Markdoc.svelte
Outdated
|
||
const ast = Markdoc.parse(doc) | ||
const configWithFrontmatter = addFrontmatter(ast, config) | ||
const content = Markdoc.transform(ast, configWithFrontmatter) |
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.
Previous three lines should probably be in a reactive block $: { .... }
. since user can change props doc
, config
, etc..
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.
100% yep
Hey @joshnuss — I've tackled most of your comments bar the idea of using |
This is a first attempt at adding
Markdoc.svelte
into the mix. I have rejigged the repo as persvelte-kit package
, but if you can think of a cleaner way to bundle the component and the preprocessor then I'd be happy to abandon this approach.These things remain on the todo list: