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

Conversation

Projects
None yet
6 participants
@smikitky
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

facebook-github-bot commented Feb 6, 2019

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 cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link
Collaborator

facebook-github-bot commented Feb 6, 2019

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

This comment has been minimized.

Copy link

reactjs-bot commented Feb 6, 2019

Deploy preview for reactjs ready!

Built with commit 16515ce

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

@tesseralis tesseralis requested review from bvaughn and gaearon Feb 6, 2019

@bvaughn

bvaughn approved these changes Feb 6, 2019

Copy link
Contributor

bvaughn left a comment

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;
};

This comment has been minimized.

@bvaughn

bvaughn Feb 6, 2019

Contributor

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

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

This comment has been minimized.

@bvaughn

bvaughn Feb 6, 2019

Contributor

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], '');
}

This comment has been minimized.

@bvaughn

bvaughn Feb 6, 2019

Contributor

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

@tesseralis

This comment has been minimized.

Copy link
Member

tesseralis commented Feb 6, 2019

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

@KyleAMathews

This comment has been minimized.

Copy link
Contributor

KyleAMathews commented Feb 6, 2019

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

This comment has been minimized.

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@smikitky

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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
You can’t perform that action at this time.