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

Support Custom-ID Markdown Syntax #1636

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Support Custom-ID Markdown Syntax #1636

merged 3 commits into from
Feb 6, 2019

Conversation

smikitky
Copy link
Member

@smikitky smikitky commented Feb 6, 2019

For the reason why this is necessary, see: #1608

This PR enables user-defined IDs on markdown headings, as explained in this guide.

A heading with an explicit custom ID would look like this:

## Try React {#try-react}

### What does const [thing, setThing] = useState() mean? {#destructuring}

To implement this, we introduce a new custom Gatsby plug-in, which will work in place of the official gatsby-remark-autolink-headers plugin (MIT Licensed).

  • Removed dependency on gatsby-remark-autolink-headers and copied most of its contents under plugins directory in this repository. The new custom plugin is called gatsby-remark-header-custom-ids.
  • Added a few lines of code to modify the AST. (e74a555)

To check how this works, edit some *.md file under content, and execute the following:

  1. yarn (removes the official plugin)
  2. yarn reset (this seems to be important after something changed in config)
  3. yarn dev or yarn build

Notes:

  • If no explicit custom ID is present, nothing changes; we just keep generating a slugified ID from the header text. So we can gradually adopt custom IDs on a per-page or per-heading basis.
  • Custom IDs must contain only lower-case alphabet, digits, hyphens and underscores.
  • This PR does not include a script to add explicit custom IDs to existing headings.

CC @tesseralis

@facebook-github-bot
Copy link
Collaborator

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link
Collaborator

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@reactjs-bot
Copy link

Deploy preview for reactjs ready!

Built with commit 16515ce

https://deploy-preview-1636--reactjs.netlify.com

@tesseralis tesseralis requested review from bvaughn and gaearon February 6, 2019 19:33
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

This looks fine to me. In hindsight, it's a great idea.

I don't mind merging this to unblock our translations efforts– but given how close it is to gatsby-remark-header-custom-ids– maybe @KyleAMathews and the Gatsby community would like to see it merged upstream?

exports.shouldUpdateScroll = ({routerProps: {location}}) => {
const offset = getTargetOffset(location.hash);
return offset !== null ? [0, offset] : true;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a forked+formatted version of gatsby-remark-autolink-headers/src/gatsby-browser.js 👍

dangerouslySetInnerHTML={{__html: script}}
/>,
]);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a formed+formatted version of gatsby-remark-autolink-headers/src/gatsby-ssr.js 👍

// Remove the custom ID part from the text node.
const lastNode = node.children[node.children.length - 1];
lastNode.value = lastNode.value.replace(match[1], '');
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a formed+formatted version of gatsby-remark-autolink-headers/src/index.js with the exception of the above ~10 lines 👍

@tesseralis
Copy link
Member

@bvaughn we could always merge it in, and switch to the open-sourced plugin later!

@KyleAMathews
Copy link
Contributor

Would love this PRed upstream! We do merge and release pretty quickly so it might be easier to just PR your improvements to us.

@bvaughn
Copy link
Contributor

bvaughn commented Feb 6, 2019

😄 I've found it pretty easy to get contributes to Gatsby plug-ins merged quickly!

I'll merge this for now and we can revert once the upstream change has been published

@bvaughn bvaughn merged commit 98c1d22 into reactjs:master Feb 6, 2019
@smikitky
Copy link
Member Author

smikitky commented Feb 7, 2019

Thank you for merging. I believe this works in our use case, but right now this is not suitable for upstream because:

  • Probably, the more "correct" way to handle this is to extend the parser. The current regexp-based approach to patch AST is hacky.
  • Only lower-cased IDs are accepted, which is a requirement specific to this website.
  • I removed tests because sibling custom plugins didn't have ones, either :)

@bvaughn
Copy link
Contributor

bvaughn commented Feb 7, 2019

I don't think I agree that the first two bullet points are blockers, and some basic tests could be written pretty easily 😄

@smikitky
Copy link
Member Author

smikitky commented Feb 7, 2019

Yes, I'll make a PR to the official plug-in tonight or this weekend :)

@bvaughn
Copy link
Contributor

bvaughn commented Feb 7, 2019

That's awesome 😄 I think other Gatsby users will really appreciate the ability to customize header ids like this

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

Successfully merging this pull request may close these issues.

6 participants