Skip to content
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

Fix rendering issue for text with unhandled generic Markdown directives #1489

Merged
merged 1 commit into from
Feb 13, 2024

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented Feb 12, 2024

What kind of changes does this PR include?

  • Changes to Starlight code

Description

This PR is a follow-up to this Discord discussion.

The remark-directive plugin used to handle asides based on generic directives is responsible of parsing 3 different types of directives:

  • text directives
  • leaf directives
  • container directives

Starlight only uses the last type but all types are parsed by the plugin. This means that content that may accidentally contain a directive of another type will be parsed as such and may lead to unexpected results. For example:

AWS re:Invent is a learning conference hosted by AWS.

The above sentence will be rendered as:

SCR-20240212-ocsm

This PR fixes the issue by transforming the unhandled directive so that they render verbatim.

SCR-20240212-ocvn

Note: this relies on mdast-util-to-markdown so that we can pass down to it the official directiveToMarkdown extension and automatically get the correct rendering for the directive including any attributes.

Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: 814f0a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Patch

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

Copy link

vercel bot commented Feb 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Feb 12, 2024 3:05pm

Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick diagnosis and fix @HiDeoo!

Left a note, but this looks good to me 🚀

Comment on lines +149 to +155
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noting a subtle difference here in the transformation back to text where the attribute value becomes wrapped in ". It should be extremely rare that this is an issue though, as it only applies in a relatively complex string that is a correctly formed directive, so I guess that’s fine?

Suggested change
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch, I totally missed that.

After a bit of digging, I found that mdast-util-directive respects options.quote altho in our case, this would not be enough to fix the issue.

I think the current approach is a good enough solution for now but if we ever hit the rare case you mentioned where we cannot restore the node to its exact original content including the quotes, we would need to revisit this. I think the 2 options would be to either:

  • Only restore the node if it's a directive with no attributes and document/emit a warning when encountering a case where we cannot restore (this seems to be the Docusaurus approach)
  • Find another way to properly restore the node (which I think would mean to have a copy of the AST before the directive was transformed)

Are you still okay with this approach?

  • If no, I can work a refactor based on the solution we would choose
  • If yes, I can merge this PR as it contains no doc changes in its current state

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, 100% approve — we can merge and release this as is.

If we run into this rare issue, we could also look into an alternative to remark-directive that only provides the container directive and not also text/lead directives.

@delucis delucis merged commit b0d36de into withastro:main Feb 13, 2024
10 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Feb 13, 2024
HiDeoo added a commit to HiDeoo/starlight that referenced this pull request Feb 20, 2024
* main: (126 commits)
  [ci] format
  [ci] release (withastro#1488)
  Fix rendering issue for text with unhandled Markdown directives (withastro#1489)
  i18n(es): update community-content (withastro#1493)
  i18n(es): update manual-setup (withastro#1492)
  Updates Korean UI translations (withastro#1487)
  i18n(fr): update `manual-setup` (withastro#1484)
  [ci] format
  i18n(ko-KR): update `manual-setup.mdx` (withastro#1482)
  i18n(ko-KR): update `community-content.mdx` (withastro#1483)
  [ci] format
  [ci] release (withastro#1481)
  [ci] format
  Make Starlight compatible with server output mode (withastro#1454)
  [i18nIgnore] community content: article description copy edit (withastro#1408)
  [ci] format
  i18n(it): Updated plugins.md and community-content.mdx (withastro#1480)
  i18n(fr): update `resources/community-content` (withastro#1479)
  [ci] format
  i18n(it): Modified everything in the /guides folder (withastro#1456)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants