-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Draft: Add an "allowHTML" feature for Markdoc #7582
Conversation
🦋 Changeset detectedLatest commit: 10e0eec The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this 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.
Overall, very impressed with the thorough documentation and unit testing. Thanks for this! I've left some comments below, and have some general comments to address:
- Folder structure: I'm not sure
html
belongs inextensions/
, since it isn't applied using theextends
configuration option. I think we can movehtml/
to the basesrc/
directory. I also think we can remove the3rd-party/
directory; a code comment at the top denoting the source should be sufficient. - Attributing MIT licensed code: Speaking with core, I don't think "taken from" is the right verbage for these comments. I'm discussing with core how we can best attribute external code. I'll follow-up with a better answer!
...rc/extensions/html/htmlattributes/mutate-raw-html-attributes-to-react-compatible-variants.ts
Outdated
Show resolved
Hide resolved
packages/integrations/markdoc/src/extensions/html/transform/html-token-transform.ts
Outdated
Show resolved
Hide resolved
Update: Here's our recommendation for citing external code!
This may seem excessive, though it makes guidelines clear for code usage. I'd use this in the files under |
This uses htmlparser2 to perform a pure token transform/mutation on the markdown-it tokens, replacing the original raw HTML string tokens with a richer set of tokens per HTML node, and in the process Markdoc tags are interleaved in the resulting token graph at the appropriate locations This removes the legacy config of the @astrojs/markdoc integration entirely (suggested by @bholmesdev) and introduces a new type for options to be specified in the astro config, initially, with just the new "enableHTML" option When "enableHTML" is *not* enabled (the default), the behavior of the entire @astrojs/markdoc integration should remain functionally equivalent to before this change
also: * cleaned up " to ' for astro project preferred linting * made the html rendering test fixture use a dynamic path
Co-authored-by: Bjorn Lu <[email protected]> Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
… definition for html
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Runtime environment variables docs for node integration * Update packages/integrations/node/README.md Co-authored-by: Reuben Tier <[email protected]> * final fixes --------- Co-authored-by: Reuben Tier <[email protected]>
* docs: add docs link to markdoc error * docs: add named exports guide to README * chore: changeset * edit: no like so Co-authored-by: Chris Swithinbank <[email protected]> * edit: exposed as named exports Co-authored-by: Chris Swithinbank <[email protected]> --------- Co-authored-by: bholmesdev <[email protected]> Co-authored-by: Chris Swithinbank <[email protected]>
I really screwed up this branch with a rebase to fix my commit author info :) I'll leave this repo up for short-term posterity but will open up a PR from a clean branch |
@alex-sherwin agh, just happened to me on a Markdoc release too. No problem, link when it's available! |
@bholmesdev New PR is ready: #7597 |
Changes
Testing
With extensive unit tests on parsed HTML DOM elements
Docs