add MDX heading permalinks for tutorial pages#1403
Conversation
| @@ -1,4 +1,4 @@ | |||
| @import "/styles/variables.scss"; | |||
| @use "/styles/variables" as *; | |||
There was a problem hiding this comment.
Was this to fix anything in particular? Ideally this change should not need to affect how we import other files, or update the package lock.
There was a problem hiding this comment.
good to know, thanks!
| type HeadingProps = HTMLAttributes<HTMLHeadingElement> & { | ||
| as: HeadingTag; | ||
| children?: ComponentChildren; | ||
| id?: string; |
There was a problem hiding this comment.
For my understanding, what passes this in? Since it seems to be implicit, we may want to add a comment to explain it.
There was a problem hiding this comment.
MDX passes these props automatically when rendering headings. Astro already injects things like id, children, etc. into the heading components, so this just forwards them into our custom component.
| aria-label="Link to this section" | ||
| > | ||
| <span class="heading-permalink__text">{children}</span> | ||
| <span class="heading-permalink__icon">#</span> |
There was a problem hiding this comment.
Not huge, but possibly this could be done with just one <a> tag as before without <span> children if we add the # via CSS? something like
.heading-permalink::after: {
content: '#';
display: inline;
/* ...etc */
}There was a problem hiding this comment.
yeah I thought about that, but since we’re making the whole heading a link, if the heading text contains inline links it could create nested anchors. that’s why I split it out earlier.
but yeah using ::after for # still makes sense.
| img: FreeRatioImage, | ||
| pre: PreProxy, | ||
| a: LinkWrapper, | ||
| h1: HeadingPermalinkH1, |
There was a problem hiding this comment.
While the tutorial layout is the main thing I had in mind when raising the issue, are there other pages that would benefit from this too? e.g. probably pages with the contributor docs should also have it. Put another way, should this be the default, maybe with other pages opting out if need be? @ksen0 let me know if you have thoughts there!
There was a problem hiding this comment.
I scoped it to tutorials first since that’s what the issue mentioned, but this could definitely be useful for other MDX pages like contributor docs too.
I wasn’t sure if it should be global by default or opt-in per layout, so I kept it limited for now. happy to extend it or move it to a shared layout if that’s preferred.
There was a problem hiding this comment.
Makes sense, we can keep it limited at first, but I think at the very least let's do this in the contributor docs pages too to start with.
There was a problem hiding this comment.
contribution_pages.mp4
I’ve extended the heading permalink behavior to contributor docs as well, while keeping it opt-in per layout rather than making it global for all markdown pages.
Tutorials and contributor docs now both use the shared heading permalink component, and the existing contributor docs jump-to links still use Astro’s generated heading slugs. I kept other MDX pages unchanged for now so we can expand deliberately if more pages need it later.
|
Thanks for working on this, your screen recording looks like what I was imagining for this feature! Left some comments, let me know what you think. |
|
Thanks for working on this! Please link the issue in the PR description |
Added the Issue Link in PR description. Please check once. |
| <a | ||
| href={`#${id}`} | ||
| class="heading-permalink__link" | ||
| aria-label="Link to this section" |
There was a problem hiding this comment.
I suggest removing this aria-label so the browser can determine the accessible name from the actual link content.
Currently, generic text like "Link to this section" will repeat for every heading link, making them indistinguishable and confusing for screen reader users.
There was a problem hiding this comment.
I am glad for your suggestion❤️. Made changes according to that, have a look into that.
There was a problem hiding this comment.
Looks good to me. Thank you.
ac1a777 to
d487cbe
Compare
| } | ||
|
|
||
| .heading-permalink__link::after { | ||
| content: "#"; |
There was a problem hiding this comment.
I suggest using alternative text syntax for the CSS-generated content to specify an empty string. This ensures screen readers do not read the character aloud:
.heading-permalink__link::after {
content: "#" / "";
}Browser support is strong across modern engines, excluding legacy IE (see Can I Use). You can verify this by inspecting the accessible name in the browser DevTools Accessibility tab to ensure the "#" symbol is excluded from the calculated link name.
For more details, see Sara Soueidan's article: https://www.sarasoueidan.com/blog/alt-text-for-css-generated-content/
Summary
Adds MDX heading permalinks for tutorial pages using component overrides in
TutorialLayout.astro.Closes #1398
Changes
HeadingPermalinkcomponent (Preact)h1–h6in tutorial MDX rendering only#fragment)#indicator appears on hover/focus (left side, subtle color).tutorials .rendered-markdownDemo
Short demo showing heading hover, permalink visibility, and fragment navigation:
https://github.com/user-attachments/assets/ab1f6c54-a9a5-423d-860e-360aec349cd9
Testing
npm run checkpasses (no errors)npm run buildsucceeds#indicator#fragmentnavigation scrolls correctlyNotes