-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
| Fix RM-XYZ |
|---|
Changes
Notes
These changes related to https://github.com/readmeio/readme/pull/17426
Issue
Built-in
When the markdown parser encounters an inline component like:
Click
It fragments it into multiple sibling nodes inside the paragraph:
paragraph
+-- text: "Click "
+-- html: "
+-- text: "here"
+-- html: "
+-- text: " to learn more."
Unlike block components (Recipe, Callout) which are parsed as single elements, inline components need to be reassembled from these fragments.
Solution
Created mdxish-inline-components.ts transformer - A new MDAST transformer that:
- Detects PascalCase inline component opening tags (e.g., )
- Finds the matching closing tag
- Preserves all attributes (href, target, title, etc.)
QA & Testing
maximilianfalco
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.
looks good but probs need to remove the duplicate JSX expression parsing
|
Logic to transform the anchor tag looks good, tests are pretty extensive, though it covers many edge cases I was concerned about. Since the new transformer only applies to tags & is behind the editor flag, this shouldn't cause rendering regressions. Just 1 concern about the evaluateExpressions code! |
kevinports
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.
Looks good. Had a few minor comments.
| }); | ||
|
|
||
| it('should handle Anchor with JavaScript URL (should preserve as-is)', () => { | ||
| // Security note: Anchor component should handle sanitization at render time |
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.
Does the Anchor component actually sanitize this?
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.
@Jadenzzz you marked this conversation as resolved, but this was an actual question for you. Does the Anchor component actually sanitize this already? Or is this work we need to follow up with?
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.
sorry this comment is kinda misleading. The goal of this test is to make sure JavaScript URLs are preserved as-is
|
@Jadenzzz is this good to merge? Then you can get on with https://github.com/readmeio/readme/pull/17426 |