-
Notifications
You must be signed in to change notification settings - Fork 2
Fix "Copy as Markdown" to copy vanilla markdown to clipboard #328
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sebastian (Tiedtke) Huckleberry <[email protected]>
| function remarkReplaceMdxComponents() { | ||
| return (tree: Root) => { | ||
| const calloutComponents = ["Note", "Warning", "Info", "Success", "Callout"]; | ||
| const otherComponents = [ |
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 seems fairly brittle. What happens when we add new components?
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.
As per comments, the AST visitor does 4 passes. Up until 3 it handles them semantically:
- Handle TabbedSection components
- Handle standalone Tab components (outside TabbedSection)
- Handle callout components (Note, Warning, Info, Success, Callout)
- Just extract content from any other components that we don't semantically understand (see 677f978)
My suggestion would be to instead error fatally with a descriptive message if 4.) comes across an unknown component. This guards against a generic "text extraction" not covering a new components semantic's appropriately.
Happy to make the change.
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.
Since there's already a "mdx linter" that runs pre-commit. It'd be low-hanging fruit to extend it to check for new "unknown" mdx components to prevent undefined behavior slipping through the cracks.
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 I think something that ensures we don't accidentally not handle something would be good
Signed-off-by: Sebastian (Tiedtke) Huckleberry <[email protected]>
Signed-off-by: Sebastian (Tiedtke) Huckleberry <[email protected]>
677f978 to
8d7f1c6
Compare
This PR "fixes" the button so it will no longer include un-rendered MDX into what's copied as shown in the screenshot below.
It's unclear if the
llms.txtshould only include plain markdown, too. According to official spec, it's plain markdown. Currently, it does include mdx tags. - I have stopped short of including a fix forllms.txtbecause it's a larger refactor and wanted to be sure it's welcomed first.