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

Generate id from heading #4434

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Conversation

andesol
Copy link
Contributor

@andesol andesol commented Apr 21, 2022

Done

  • Search for h2s without an id and generate one from the heading.

(I think in HTML5 any character is allowed for ids except for whitespace, so I kept it simple. Just lower cased it for consistency)

QA

  • Open demo
  • In the docs, go to the the design guidelines of the "Segmented Control" (or "Get Started", for the matter)
  • Check side nav is generated from the headings

Screenshots

image

@webteam-app
Copy link

Demo starting at https://vanilla-framework-4434.demos.haus

@@ -97,7 +103,7 @@
var anchor = document.createElement('a');
anchor.classList.add('p-side-navigation__link');

// Add all H3s with IDs to the table of contents list
// Add all H2s with IDs to the table of contents list
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, thanks for reading the comments ;)

@@ -87,6 +87,12 @@

// Add table of contents to side navigation on documentation pages
(function () {
// Generate id from H2s content when it does not exist
document.querySelectorAll('main h2:not([id])').forEach(function (heading) {
var id = heading.textContent.toLowerCase().replaceAll(/\s+/g, '-');
Copy link
Contributor

Choose a reason for hiding this comment

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

This may fail with some special characters in headers.

Maybe worth adding some more rules in here. That's what I found in one markdown implementation:

https://github.com/markedjs/marked/blob/30e90e5175700890e6feb1836c57b9404c854466/src/Slugger.js#L19

Copy link
Contributor

@bartaz bartaz left a comment

Choose a reason for hiding this comment

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

LGTM, with a little suggestion to make it more bulletproof

@andesol andesol merged commit 522fa4f into canonical:new-vanilla-website Apr 21, 2022
@andesol andesol deleted the generate-ids branch April 21, 2022 15:38
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.

None yet

3 participants