-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(gatsby-remark-autolink-headers): Support {#custom-id} header syntax #14253
Conversation
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.
Great! I know (or thought I know) that reactjs.org uses an approach kind of like this--it'd be nice to bake this into the plugin so everyone can get this benefit, so thanks for taking the initiative here!
Left a few comments/requests 👇
visit(transformed, `heading`, node => { | ||
expect(node.data.id).toBeDefined() | ||
|
||
expect(node).toMatchSnapshot() |
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.
Sort of a nit: but generally I'd argue we way overuse snapshots and they make future failures more confusing than they need to be.
Could you rather test for a specific id, e.g. something like
const customId = `custom-id`
const markdownAST = remark.parse(`
# Heading With a Custom Id {#${customId}}
`)
// later
visit(transformed, `heading`, node => {
expect(node.data.id).toBe(customId)
})
Obviously you can scale this however you'd like if there's value in testing for multiple instances.
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.
I agree. I rewrote this test without using snapshots. Does it look better? If you like this, I will rewrite other tests with this style in another PR.
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.
I have added a question as @DSchau is busy spreading the word about gatsby on a conference. This looks pretty good!
Could you also add 3 tests? 1 to check we still generate an id when no {#id}
is specified, a second one to check that we don't convert headings with only the {#id}
as text and a last one to test that we don't use the custom id from this node {#id} some text
Added additional tests. Could you take another look? |
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.
Well done! 👏 Thanks for addressing all comments. Let's ship this! 🚀
Holy buckets, @yunabe — we just merged your PR to Gatsby! 💪💜 Gatsby is built by awesome people like you. Let us say “thanks” in two ways:
If there’s anything we can do to help, please don’t hesitate to reach out to us: tweet at @gatsbyjs and we’ll come a-runnin’. Thanks again! |
published in |
Thank you for the reviewing, Ward. |
…tax (gatsbyjs#14253) - add enableCustomId option - enable usage of custom-id ``{#custom-id}` when enableCustomId is set to true
Description
Support custom heading ID syntax in gatsby-remark-autolink-headers (disabled by default)
https://www.markdownguide.org/extended-syntax/#heading-ids