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

feat(v2): add ability to set custom heading id #4222

Merged
merged 15 commits into from
Mar 5, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 13, 2021

Motivation

Closes #3322

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tests and preview https://deploy-preview-4222--docusaurus-2.netlify.app/classic/examples/markdownPageExample#custom

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: new feature This PR adds a new API or behavior. label Feb 13, 2021
@lex111 lex111 requested a review from slorber as a code owner February 13, 2021 01:25
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 13, 2021
@netlify
Copy link

netlify bot commented Feb 13, 2021

[V1] Deploy preview success

Built with commit c8baed6

https://deploy-preview-4222--docusaurus-1.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 13, 2021

Size Change: +543 B (0%)

Total Size: 533 kB

Filename Size Change
website/build/assets/css/styles.********.css 87.4 kB +51 B (0%)
website/build/assets/js/main.********.js 359 kB +492 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/build/blog/2017/12/14/introducing-docusaurus/index.html 60.3 kB 0 B
website/build/docs/introduction/index.html 235 B 0 B
website/build/index.html 25.4 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Feb 13, 2021

Deploy preview for docusaurus-2 ready!

Built with commit c8baed6

https://deploy-preview-4222--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 13, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 92
🟢 Accessibility 96
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-4222--docusaurus-2.netlify.app/classic/

@netlify
Copy link

netlify bot commented Feb 13, 2021

[V1] Deploy preview success

Built with commit 728d262

https://deploy-preview-4222--docusaurus-1.netlify.app

@netlify
Copy link

netlify bot commented Feb 13, 2021

Deploy preview for docusaurus-2 ready!

Built with commit 728d262

https://deploy-preview-4222--docusaurus-2.netlify.app

// Support explicit heading IDs
const customHeadingIdMatches = customHeadingIdRegex.exec(heading);

if (customHeadingIdMatches) {
Copy link
Contributor Author

@lex111 lex111 Feb 13, 2021

Choose a reason for hiding this comment

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

Curious if this feature will have to be configurable? How much of an impact on performance would it have if it were always enabled by default? Or does it make sense to create a separate plugin for this feature? For now I have combined it into the existing slug plugin because they essentially do the same thing (create headings id).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to enable by default and put in mdx loader.
We likely want to encourage users to have stable anchor ids and this should work consistently for docs, blog and pages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just thought that having new plugin might be better for this. Since this feature is mainly needed when using the i18n. Moreover, I think to add a cli command to automatically create explicit ids for existing headers (we have extendCli hook in plugin lifecycle). Because currently I do not understand how I can add a new cli command inside the mdx-loader package. WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a way to "lock" the ids at a given time looks like a nice idea.

Not sure how to build that easily, how would the plugin know where to look for markdown files, as the paths are provided as options to the docs, blog, page plugin?

The only thing I'm thinking of is having this cli in core, and getting all the relevant paths from getPathsToWatch (as you basically watch the md files, it will work but feel a bit hacky at the same time)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this feature is mainly needed when using the i18n.

I think it's actually useful in all cases. If you want to link to some heading, we'd rather ensure that link does not break when the heading is updated.

Fixing heading ids you want to link to should rather be the norm, and we could even add a warning someday to tell the user that he's linking to an anchor that has not been fixed?

Currently it's too easy to have broken anchor links in a Docusaurus site.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lost sight of that it is currently not possible to add new Remark plugin from the custom Docusaurus plugin. Maybe we should create new hook for this purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to add new command to write heading ids to the docusaurus core (eg. yarn docusaurus write-heading-ids community). Its implementation is very simple, so it might be worth improving it.

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, that will be really useful for i18n!


const visit = require('unist-util-visit');
const toString = require('mdast-util-to-string');
const slugs = require('github-slugger')();

const customHeadingIdRegex = /^(.*?)\s*\{#([\w-]+)\}$/;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about extracting customHeadingIdRegex.exec(heading); in a separate function const {heading,id} = extractHeadingId(rawHeading) and adding tests?

That would simplify the testing of Regepx edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, but we have separate test that with different cases, so it seems to me redundant extract new function (and its test), because in many ways it will duplicate this test.

// Support explicit heading IDs
const customHeadingIdMatches = customHeadingIdRegex.exec(heading);

if (customHeadingIdMatches) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's fine to enable by default and put in mdx loader.
We likely want to encourage users to have stable anchor ids and this should work consistently for docs, blog and pages.


### Custom id headers {#custom-id}

With `{#custom-id}` syntax you can set your own header id.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably worth creating a page dedicated to anchor links in markdown features category?

I'd also document this in the i18n section but I can do it in another PR?

@slorber slorber merged commit 96e7fce into master Mar 5, 2021
@krillboi
Copy link

@slorber I just tried this out by calling ./yarn --cwd ../../../documentation/docusaurus-site docusaurus write-heading-ids but nothing seems to happen and I just get a 0 markdown files already have explicit heading ids message. Am I using this incorrectly?

@slorber
Copy link
Collaborator

slorber commented Mar 10, 2021

@krillboi can you try on the doc site folder directly instead of using --cwd ?
Are you using Yarn2?

@krillboi
Copy link

@slorber I can try that. I am currently using yarn v1.22.10.

@krillboi
Copy link

@slorber I tried this instead, executed from the site directory: ../../support/3rdparty/nodejs12140/yarn docusaurus write-heading-ids and it still does not work.

@slorber
Copy link
Collaborator

slorber commented Mar 10, 2021

😅 I mean can you try cding into the directory and just running yarn docusaurus write-heading-ids ?

What's your site repo? a branch to inspect?

@krillboi
Copy link

krillboi commented Mar 10, 2021

@slorber I don't have a global installation of node. Haven't had trouble with using --cwd until now for all the other docusaurus commands like write-translations, start, build etc. I don't have a public repo I'm afraid 😅

If I write some invalid path to siteDir like: ../../support/3rdparty/nodejs12140/yarn docusaurus write-heading-ids ../ I correctly get a Error: Config file "c:\git\touchgfx.git\documentation\docusaurus.config.js" not found error, but the other command seems to find the config file fine, but does not do anything to my .mdx files:

cd documentation/docusaurus-site
../../support/3rdparty/nodejs12140/yarn docusaurus write-heading-ids
yarn run v1.22.10
$ c:\git\touchgfx.git\documentation\docusaurus-site\node_modules\.bin\docusaurus write-heading-ids
0 markdown files already have explicit heading ids
Done in 3.81s.

Is it recommended to upgrade to yarn 2?

@krillboi
Copy link

krillboi commented Mar 10, 2021

So I've tried logging the output of the command and

const markdownFiles = await globby(await getPathsToWatch(siteDir), {
returns an empty array, so it isn't finding my .mdx files. I'll investigate further.

@krillboi
Copy link

Still haven't fixed it but have a suspicion that it has something to do with: sindresorhus/globby#155

@krillboi
Copy link

@slorber So replacing the globby call with the following has worked for me (taking inspiration from some of your other functions):

    const markdownFiles = await globby_1.default.sync(['**/*.{md,mdx}'], {
        cwd: siteDir,
    });

It's not perfect as, for example, all the README.md files in node_modules are hit which I can see is what getPathsToWatch is trying to prevent. So I am not entirely sure what is going on, but probably Windows+globby related.

@slorber
Copy link
Collaborator

slorber commented Mar 10, 2021

Oh, I see you are on windows, that definitively look like the reason :)

thanks for reporting

This was referenced Mar 11, 2021
@slorber
Copy link
Collaborator

slorber commented Mar 16, 2021

@krillboi should be fixed by #4444

Please test this on @canary release soon and let me know if it works

@slorber slorber deleted the lex111/custom-heading-ids branch August 17, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: new feature This PR adds a new API or behavior.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to set heading anchor link id (useful for i18n)
4 participants