-
Notifications
You must be signed in to change notification settings - Fork 1.1k
In-page nav - Add customizable data- attribute for which heading[s] gets queried #5034
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just a question on the new data-headings
setting.
`${MAIN_ELEMENT} h2, ${MAIN_ELEMENT} h3` | ||
); | ||
const getSectionHeadings = (headings) => { | ||
const headingList = headings.indexOf(" ") ? headings.split(" ") : headings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this headings.indexOf(" ")
to check for multiple values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we validate any of the values passed into data-headings
? Or at least return an error if a user tries to pass an invalid header ex: j1 j2 instead of h1 h2 ?
For example, validate against a list of expected values? Could be a const VALID_HEADINGS = ["h1", "h2", "h3", "h4", "h5", "h6"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this
headings.indexOf(" ")
to check for multiple values?
Yes, do you think this should include validation against, for example, if they use a dash instead of a space for multiple headings? We could do an IN_PAGE_NAV_VALID_DELIMITERS array like [" ", ",", "-"]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That might not be necessary, but that's a good question. I feel like it's good enough to throw an error once the expected heading element is invalid.
FWIW; I had thought something similar like passing a value [h1, h2, h3]
to the data-attribute for clarity. Unfortunately, it doesn't make things any easier for anyone.
Holding on this one, pending further discussion |
Moving this work into #5444. Thanks for getting this started, @mitchmoccia! |
This implements the feature detailed here:
#5030
This is an important one that will allow for the customization of what headings should be queried when the in-page nav is being dynamically built.
A developer will be able to easily add the data- attribute
data-headings
to the aside element in the HTML to define which headings get queried.Some examples include:
data-headings="h2"
data-headings="h2 h3"
data-headings="h2 h3 h4"
data-headings="h1 h2"
The headings should be listed with a space in between.
NOTE: This is most likely needed ASAP for the USWDS documentation site since including all h3s in the query is problematic on some of the pages... It might be better to use
data-headings="h2"
so only h2s are built in the in-page nav. Let's discuss.