Skip to content

Use an h5 for callouts#27648

Merged
XhmikosR merged 2 commits intov4-devfrom
v4-dev-xmr-docs-access-callout
Nov 27, 2018
Merged

Use an h5 for callouts#27648
XhmikosR merged 2 commits intov4-devfrom
v4-dev-xmr-docs-access-callout

Conversation

@XhmikosR
Copy link
Copy Markdown
Member

@XhmikosR XhmikosR commented Nov 11, 2018

This is so that they are not included in ToC.

@XhmikosR
Copy link
Copy Markdown
Member Author

XhmikosR commented Nov 11, 2018

Now that I think about it... The header levels for some of the includes don't make sense.

https://deploy-preview-27648--twbs-bootstrap4.netlify.com/docs/4.1/utilities/colors/#conveying-meaning-to-assistive-technologies

They show as children in ToC.

The thing is we can't know the level of the previous sibling item and thus use the same level in the callouts.

So, perhaps we should just hide the callouts' headers consistently?

@patrickhlauke
Copy link
Copy Markdown
Member

nobody dies if a heading level is skipped. unless we have a way to embed callouts at any level, and the callout auto-adapts the level to be subordinate of the last heading level it's at, it's safest to keep the heading level to something guaranteed to be subordinate in every situation...so let's leave it to h5, or heck could even go to h6. whether it's included in the TOC or not is up for debate.

@XhmikosR
Copy link
Copy Markdown
Member Author

The reason for this PR wasn't the skipped level. Was to show the callout in the ToC. But after thinking about it, I believe it's better if we just hide all callouts instead from ToC.

Copy link
Copy Markdown
Member

@patrickhlauke patrickhlauke left a comment

Choose a reason for hiding this comment

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

if we can guarantee that the callout is only ever included with a preceding h3, and not an h4 or h5, then sure. but i think it's not a major problem even as is.

@patrickhlauke
Copy link
Copy Markdown
Member

ah gotcha,hadn't seen your comment until now. yeah, i think it's fine even if the callouts AREN'T in the TOC

@XhmikosR
Copy link
Copy Markdown
Member Author

All right, I'm gonna hide them by using an h5.

@XhmikosR XhmikosR changed the title Use an h4 for the accessibility callout Use an h5 for callouts Nov 11, 2018
@XhmikosR XhmikosR removed the on-hold label Nov 11, 2018
@XhmikosR
Copy link
Copy Markdown
Member Author

Actually, it might be a bug in jekyll-toc. Waiting for confirmation/fix toshimaru/jekyll-toc#70

@m5o
Copy link
Copy Markdown
Contributor

m5o commented Nov 14, 2018

@XhmikosR Consider review/cherry-pick 08b096d from #25838 this should cover the whole list.

@XhmikosR
Copy link
Copy Markdown
Member Author

Thanks but I want this fixed upstream.

@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-access-callout branch from 233f175 to 9e74183 Compare November 20, 2018 10:36
@mdo
Copy link
Copy Markdown
Member

mdo commented Nov 26, 2018

FWIW, I don’t want callouts to be listed in the table of contents. Any of them that are there currently are there unintentionally.

@XhmikosR
Copy link
Copy Markdown
Member Author

Yeah, I think I will just change them so that they don't show up for now. No idea when this will be fixed upstream.

@XhmikosR XhmikosR removed the on-hold label Nov 27, 2018
@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-access-callout branch from 1fdef2d to f6cda5e Compare November 27, 2018 12:59
Callouts are already excluded from ToC, but due to a limitation in jekyll-toc they are still being included.

We should revisit this if the bug is fixed later.
@XhmikosR XhmikosR force-pushed the v4-dev-xmr-docs-access-callout branch from f6cda5e to 7074a2d Compare November 27, 2018 13:06
@XhmikosR XhmikosR merged commit 11da160 into v4-dev Nov 27, 2018
@XhmikosR XhmikosR deleted the v4-dev-xmr-docs-access-callout branch November 27, 2018 14:01
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.

5 participants