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: fixes a validation issue with Markdown headings including trailing markup #72

Merged
merged 6 commits into from
Dec 2, 2024

Conversation

jorenbroekema
Copy link
Contributor

Describe the pull request

This PR adds a test for having various types of markup inside a heading and validating that you can still link to those.

Why

There is a small gap between how Astro renders the id attributes of headings (slugs) and what the GitHub Slugger produces, and this gap leads to false-positives in this validation utility.

How

I decided to dig into why this happens, it seems like it's just the trailing hyphen that's the problem so I added a one line normalisation step to close the gap between Astro's heading id attribute and GitHub Slugger.
I would totally understand if you feel like this proposed fix is not the right approach and instead we should be contributing a fix to either Astro or Github Slugger to ensure this gap between slug outputs doesn't exist in the first place, rather than normalising it here. I'd just need a bit of direction on where to contribute this fix, if it shouldn't be in this project.

As for possible workarounds, there isn't one that I can think of besides patching this utility locally. I tried appending a trailing hyphen to my links that link to headings that I know have trailing markup, it will consider it valid from the perspective of this validation util but it won't work when you click this link on the actual page, because it doesn't match the rendered DOM id attribute of the heading.

Screenshots

Below screenshot shows:

  • a heading with a <Badge> markup
  • rendered id attr of the heading being "exportplatform"
  • validation util console output false-positive error for links that reference this heading

image

Copy link

vercel bot commented Nov 29, 2024

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

Name Status Preview Comments Updated (UTC)
starlight-links-validator ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 2, 2024 10:17am

@HiDeoo
Copy link
Owner

HiDeoo commented Nov 29, 2024

Thanks for your contribution 🙌

First, just to double check, am I correct that the PR title is wrong and is not related to extra slash but hyphen?

I didn't get the chance yet to play with the issue but I think the issue is located in this plugin and like you mentioned when we're generating the slug based on the content. Altho, I don't think eliminating the hyphen would be the best solution but instead eliminating the source of the hyphen which I think is just the plugin not trimming trailing spaces on the content like it should.

In my head (and again, may be totally wrong as I did not test before writing yet), fileHeadings.push(slugger.slug(content.trimEnd())) should be enough to fix the issue. Would you be able to give it a try?

Copy link
Owner

@HiDeoo HiDeoo left a comment

Choose a reason for hiding this comment

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

In my head (and again, may be totally wrong as I did not test before writing yet), fileHeadings.push(slugger.slug(content.trimEnd())) should be enough to fix the issue.

Turns out I was wrong and your approach is totally correct as it matches the one used by Astro.

I took the liberty of updating the PR with the following changes so we can merge it right away:

  • I removed the new fixture, I try to only create new ones when necessary, e.g. a different Astro configuration
  • I reverted the pnpm lockfile to the original one as it was no longer necessary and the pushed version was also changing the lockfile version
  • I slightly updated one of the example you provided where my suggestion was wrong and only your approach is correct
  • I added a comment with a link to the exact Astro code that uses the same approach when extracting heading IDs

Thanks again for the amazing contribution 🙌

@HiDeoo HiDeoo changed the title fix: trim trailing slash from slug for heading w markup fix: fixes a validation issue with Markdown headings including trailing markup Dec 2, 2024
@HiDeoo HiDeoo merged commit 009be12 into HiDeoo:main Dec 2, 2024
4 checks passed
@jorenbroekema
Copy link
Contributor Author

Ah yeah I meant hyphen and not slash oops :D

Thanks for finishing and merging the PR, much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants