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

Handle titles starting with numbers #13

Closed
4 tasks done
chadfawcett opened this issue Oct 24, 2022 · 4 comments
Closed
4 tasks done

Handle titles starting with numbers #13

chadfawcett opened this issue Oct 24, 2022 · 4 comments
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on

Comments

@chadfawcett
Copy link

Initial checklist

Problem

Note: Not sure if this is better solved here or in github-slugger. I decided to open here since I think a solution to this problem would make that package stray from it's intention of being as close to GitHub's slugger as possible. Happy to move over to that repo if you think it's a better fit there though.

I have a scenario where an author of some content has started the header with a number. This results in an id that starts with a number, which isn't valid according to the CSS spec (HTML spec was a little less clear whether it was valid or not). Regardless, when I tried to reference the header using document.querySelector, it throws an error saying I'm using an invalid selector.

Solution

A solution I could take on my end would be to append something to the beginning of these IDs and update the links, but I thought it'd be better to be done upstream (either in rehype-slug or github-slugger).

I created a quick solution for github-slugger where it just appends an underscore (_) to any id that'd start with a number. I figured I should reach out first before submitting a PR though. GitHub appends user-content- to all of the ids of in the rendered markdown. You can see this when inspecting the README of this project.
Screen Shot 2022-10-24 at 1 58 03 PM

This is where I'm unsure of where this solution should be implemented. GitHub doesn't include the user-content- in the url produced for each header, but instead handles that client side. So github-slugger only sluggifies what comes after the user-content-. So if we start appending something like an underscore, it means that package may deviate too much from its goal.

For those reasons, I think it may be the responsibility of this package.

Alternatives

I touch on the alternatives in the Solutions section above, but I'll reiterate and summarize here:

  1. Handle it in my specific application
  2. Handle it in rehype-slug as I think this could be a problem for many people (not just me)
  3. Handle it in github-slugger if it doesn't force that project to deviate from it's goal of "emulate the way GitHub handles generating markdown heading anchors as close as possible."
@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Oct 24, 2022
@wooorm
Copy link
Member

wooorm commented Oct 25, 2022

Hey Chad!

HTML spec was a little less clear whether it was valid or not

Starting an id attribute value with a number is valid according to HTML. It’s fine.
It only becomes a problem when trying to select it with a selector shortcut: querySelector('#3-xxx') or #3-xxx { color:red }.
It is fine with document.getElementById('3-xxx') and also fine with document.querySelector('[id="3-xxx"]') / [id="3-xxx"] { color: red }

GitHub appends user-content- to all of the ids of in the rendered markdown.

GitHub doesn't include the user-content- in the url produced for each header, but instead handles that client side

Yep, and it does so to prevent DOM clobbering, as a safety measure.
That’s why we also do it at a different place in the pipeline: rehype-sanitize. It has a whole section on how that prefix works, what it solves, and what GitHub adds client-side: https://github.com/rehypejs/rehype-sanitize#example-headings-dom-clobbering.

I’m open to adding an option here to prefix things.
Alternatively, you could investigate using rehype-sanitize too.
But also: perhaps using attribute syntax ([id="3-xxx"]) or getElementById might solve it for you?

@chadfawcett
Copy link
Author

Thanks for the detailed response @wooorm!

rehype-sanitize definitely seems like the way to go for most projects. Since this is an internal project, I'm going to just go with the getElementById/[id="3-xxx"] syntax so I don't have to worry about the caveats with the changing classes for the other plugins (linking, syntax highlighting, etc).

I’m open to adding an option here to prefix things.

I think the two solutions you provided resolve the need for this option. Hopefully this issue can help someone in the future!

Thanks again :)

@github-actions

This comment has been minimized.

@wooorm wooorm added 👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on labels Oct 26, 2022
@wooorm
Copy link
Member

wooorm commented Oct 26, 2022

Cool, glad those alternatives work for you!

@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Oct 26, 2022
wooorm added a commit that referenced this issue Oct 31, 2022
Related-to: GH-13.
Closes GH-14.
Closes GH-15.

Reviewed-by: Titus Wormer <tituswormer@gmail.com> 

Co-authored-by: Titus Wormer <tituswormer@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
👀 no/external This makes more sense somewhere else 👎 phase/no Post cannot or will not be acted on
Development

No branches or pull requests

2 participants