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

Replace AnchorJS with a Hugo render hook #32953

Merged
merged 6 commits into from
Feb 19, 2022
Merged

Conversation

XhmikosR
Copy link
Member

@XhmikosR XhmikosR commented Feb 1, 2021

TODO:

  • move the levels to config.yml
  • a11y
  • maybe use ::after?
  • add padding?
  • fix :focus if possible
  • test that there are no regressions

Idea taken from gohugoio/gohugoioTheme#146

Pros:

  • No JS
  • ...

Cons:

  • Works only with Markdown headings, but on the other hand our ToC only works with Markdown already
  • ...

Preview: https://deploy-preview-32953--twbs-bootstrap.netlify.app/docs/5.1/getting-started/introduction/

@XhmikosR XhmikosR added the docs label Feb 3, 2021
@XhmikosR
Copy link
Member Author

XhmikosR commented Feb 8, 2021

@mdo @ffoodd any thoughts?

@XhmikosR XhmikosR force-pushed the main-xmr-docs-render-heading branch from 4614292 to 86d5d5e Compare August 20, 2021 15:31
@coliff
Copy link
Contributor

coliff commented Aug 20, 2021

heya - FYI- I used render hook code similar to this for a docs project and Algolia displayed the anchor # in search results window after the titles which wasn't ideal.
We changed it so that the # was displayed using ::after like you noted - so would definitely recommend you do that for a nicer Algolia search UX.
But other than that - I would recommend this approach. It works great and nice to not need JavaScript :-)

@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Sep 9, 2021
@mdo mdo removed this from In progress in v5.2.0 Nov 2, 2021
@mdo mdo added this to In progress in v5.3.0 via automation Nov 2, 2021
@XhmikosR XhmikosR removed this from In progress in v5.3.0 Nov 30, 2021
@XhmikosR XhmikosR added this to In progress in v5.2.0 via automation Nov 30, 2021
@XhmikosR XhmikosR requested a review from a team December 3, 2021 08:42
@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 9, 2021

@ffoodd could you help me out with this please? ❤️

@ffoodd
Copy link
Member

ffoodd commented Dec 9, 2021

I'll try to 👌

Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Seems already good to me!

What :focus fix did you have in mind? It's working fine IMHO.
There's no need to use a pseudo-element to handle # here because aria-label erases text content from link's accessible name.

So yeah, that's really good I think :)

site/assets/scss/_anchor.scss Outdated Show resolved Hide resolved
site/assets/scss/_anchor.scss Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
<h{{ .Level }} id="{{ .Anchor | safeURL }}">{{ .Text | safeHTML }}
{{- if and (ge .Level 2) (le .Level 5) }}{{" " -}}
<a class="anchor-link" href="#{{ .Anchor | safeURL }}" aria-label="Anchor">#</a>
Copy link
Member

Choose a reason for hiding this comment

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

aria-label value is questionable. @patrickhlauke Do we consider the heading context is enough, or should we try to complete the label using heading's content?

Copy link
Member

Choose a reason for hiding this comment

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

yeah i'd probably go for something a bit more descriptive, like having aria-label="Link to this section: {{ .Text | safeHTML }}" perhaps. yes, admittedly a bit wordy, but it front-loads what the meaning of the link is and AT users can always choose not to listen to the entire thing

Copy link
Member Author

Choose a reason for hiding this comment

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

Note that I used what anchor.js is already doing. I don't have a preference about this, whatever you guys think is better.

@ffoodd
Copy link
Member

ffoodd commented Dec 16, 2021

Regarding this being markdown headings only: since this is outputs a very simple markup, we should be able to add anchors manually on markup headings, shouldn't we? It may even be a simple shortcode to help computing the markup.

@XhmikosR
Copy link
Member Author

XhmikosR commented Dec 17, 2021

Seems already good to me!

What :focus fix did you have in mind? It's working fine IMHO. There's no need to use a pseudo-element to handle # here because aria-label erases text content from link's accessible name.

So yeah, that's really good I think :)

See @coliff's comment above.

Regarding :focus, I thought that we showed the anchor on focus but apparently we don't.

Regarding this being markdown headings only: since this is outputs a very simple markup, we should be able to add anchors manually on markup headings, shouldn't we? It may even be a simple shortcode to help computing the markup.

I think we should be OK in the sense that ToC already works with Markdown headings anyway. So, this way, we are actually more consistent, I think.

@XhmikosR XhmikosR marked this pull request as ready for review January 30, 2022 14:53
@XhmikosR XhmikosR requested a review from a team as a code owner January 30, 2022 14:53
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

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

Made a few tweaks to polish this: improved aria-label after discussion, and (opinionated) made the anchor link visible when anchor is active (with :target).

Might help recalling the current URL targets an anchor, which I often forget about :D

Edit: also moved the # to a pseudo-element to follow @coliff 's recommendation.

v5.2.0 automation moved this from In progress to Reviewer approved Feb 8, 2022
@XhmikosR XhmikosR merged commit ae12d64 into main Feb 19, 2022
v5.2.0 automation moved this from Reviewer approved to Done Feb 19, 2022
@XhmikosR XhmikosR deleted the main-xmr-docs-render-heading branch February 19, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5.2.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants