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

Heading anchor slug does not respect GitHub behavior due to emojis #9194

Closed
slorber opened this issue Aug 3, 2023 Discussed in #9193 · 3 comments · Fixed by #9202
Closed

Heading anchor slug does not respect GitHub behavior due to emojis #9194

slorber opened this issue Aug 3, 2023 Discussed in #9193 · 3 comments · Fixed by #9202
Labels
bug An error in the Docusaurus core causing instability or issues with its execution good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin.

Comments

@slorber
Copy link
Collaborator

slorber commented Aug 3, 2023

Heading anchor slug does not respect GitHub behavior due to emojis

Discussed in #9193

Considering the heading ## :smiley: This is a friendly header

Our slugger package is supposed to align with GitHub and seems to work as intended (the input heading string is not supposed to contain the leading space):

    expect(slugger.slug(' This is a friendly header')).toBe(
      '-this-is-a-friendly-header',
    );
    expect(slugger.slug('This is a friendly header')).toBe(
      'this-is-a-friendly-header',
    );
    expect(slugger.slug(' :smiley: This is a friendly header')).toBe(
      '-smiley-this-is-a-friendly-header',
    );
    expect(slugger.slug(':smiley: This is a friendly header')).toBe(
      'smiley-this-is-a-friendly-header',
    );

    // Yes, that's how GitHub behave in this case
    expect(slugger.slug('😃 This is a friendly header')).toBe(
      '-this-is-a-friendly-header',
    );

The MDX Playground shows the heading is parsed this way, and it's the value we pass to our slugger:

{
      "type": "heading",
      "depth": 2,
      "children": [
        {
          "type": "text",
          "value": ":smiley: This is a friendly header"
        }
      ]
    }

What I understand:

We should probably use node-emoji to "unconvert the value" before passing it to the slugger.

import * as emoji from https://github.com/omnidan/node-emoji,

slugger.slug(emoji.unemojify('😃 This is a friendly header'));

The problem is that we shouldn't always unemojify 😅

If you look at the GitHub behavior, it has a different slugging behavior if we use 😃 or :smiley: in a heading:
See https://gist.github.com/slorber/9c499bb934434a3f7ac6603eff6647d3#-this-is-a-friendly-header-real-smiley
We should align to this behavior, but I'm not sure how 😅

The solution would be to get the "original" heading string value before remark-emoji gets applied. I'm not sure remark-emoji outputs that original value currently.

@slorber slorber added the bug An error in the Docusaurus core causing instability or issues with its execution label Aug 3, 2023
@Josh-Cena
Copy link
Collaborator

Why don't we just make the heading ID addition happen before emojifying?

remarkPlugins: [emoji, headings, toc],

@slorber
Copy link
Collaborator Author

slorber commented Aug 3, 2023

Ah yes, that actually looks like a good solution 🤦‍♂️ 🤪 👍

Marked as good first issue in case someone want to give it a try: just submit a PR directly.

Please add a dogfood page on our own site so that we can verify the behavior on the PR deploy preview.

@slorber slorber added the good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin. label Aug 3, 2023
@yosukekato165
Copy link
Contributor

@slorber @Josh-Cena
Please review when you have time.
#9202

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An error in the Docusaurus core causing instability or issues with its execution good first issue If you are just getting started with Docusaurus, this issue should be a good place to begin.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants