Support external navigation links #876

SPGoding · 2022-06-29T18:58:41Z

Based on #238, resolves #66.

mattxwang

Hi @SPGoding, thanks for the contribution - appreciate you taking this up.

I have two small requests:

Some in-code comments (tagged)
The icon seems a bit off-center; I'm on macOS with Chrome, and I see something like this:

Could you shift the icon down a little bit with CSS, and/or align the bottom with the font line?

Let me know what you think!

_layouts/default.html

mattxwang · 2022-07-05T00:06:31Z

_includes/nav.html

+    <li class="nav-list-item external">
+      <a href="{{ node.url | absolute_url }}" class="nav-list-link external">
+        {{ node.title }}
+        {% unless node.hide_icon %}<svg viewBox="0 0 24 24"><use xlink:href="#svg-external-link"></use></svg>{% endunless %}


Since this icon doesn't add any semantic meaning, we should hide it from a screenreader. I think in this case, we want to add

Suggested change

{% unless node.hide_icon %}<svg viewBox="0 0 24 24"><use xlink:href="#svg-external-link"></use></svg>{% endunless %}

{% unless node.hide_icon %}<svg aria-hidden="true" viewBox="0 0 24 24"><use xlink:href="#svg-external-link"></use></svg>{% endunless %}

For more, this blog post by CSS Tricks is a good summary.

Nice points about accessibility. Correct me if I'm wrong -- but doesn't this icon convey that this is an external link, thus has semantic meaning?

You know what - you're completely right, that slipped my mind. Hm, in that case, I think having some screenreader-only/aria text that says "(external link)" would make the most sense.

It seems like there's no consensus on exactly what it should say. If you have a suggestion instead of "(external link)", please go for it instead!

Precedent:

W3 WCAG20 spec saying that advanced warning is necessary

deque checklist on links

WebAIM article

Thanks for the response! The <title> element can also be used to provide a text description, which would be shown as a tooltip on most browsers. I think that tooltip could be beneficial for non-screenreader users as well, so I will instead have <title>(external link)</title> inside the <symbol> definition, and have an aria-labelledby attribute on the <svg> tag to reference that title text.

…l-links

SPGoding · 2022-07-05T19:44:17Z

This feature is more complicated than I originally thought >_<

If I understand the accessibility standards correctly, there are two types of links we may need to take care of here:

Links that point to external sites. These links MAY indicate that they are external.
Links that open in new tab. These links SHOULD only be used when necessary, and SHOULD have advance warning that they open in new tabs.

There are several cases I could think of where a user may want to add an external link:

To link to a page of the same site that's not provided by the Jekyll setup. This isn't external in the sense of "external site" but "external of Jekyll", so there should probably be no icons or any viewable differences between this link and other auto-generated links.
To open an external site in the same tab. The user should have the option of either hiding the icon or showing the icon with an aria label along the lines of "external site".
To open an external site in a new tab. The icon should be mandatory* and have an aria label along the lines of "opens in new tab".

* The "mandatory" should still just be an advice, as users can choose to write something like (new tab) in the link text themselves to fulfill the accessibility requirements.

I'm not sure what's the best way to set up the configuration options to allow users of just-the-docs to configure it according to their needs -- adding three options (new_tab: boolean, show_icon: boolean, and icon_label: string) for each link item in the config would be powerful enough but might be too much.

Another issue I see is that we only have one icon that we could use to indicate either "external site" or "opens in new tab", which may be a source of ambiguity.

mattxwang · 2022-07-06T04:43:08Z

Really appreciate the thought you've put into this! A bit swamped with work and other personal obligations but I'll be back with a review/more thoughts over the next few days!

mattxwang · 2022-07-12T22:38:50Z

Coming back to open PRs now. Thanks again for your detailed report @SPGoding; some thoughts:

let's keep the scope of this PR small. I don't think we should deal with the logic (or accessibility features) around links that open in a new tab in this PR; for now, just supporting external navigation suffices as a good feature!
if you're up for it, we can certainly open a new issue to discuss changing the default behaviour for links that open in a new tab - the configuration options you've suggested sound reasonable to me, and I'm sure other users/maintainers would find this functionality useful

What do you think?

(testing locally, I think the link alignment is better, and the title label does work - no other complaints!)

SPGoding · 2022-07-13T04:59:21Z

Thanks for your feedback!

I agree with keeping the scope of this PR small, and I created #881 to keep track the accessibility concerns around links that open in new tabs.

mattxwang

Sounds good. Thanks for your quick response and your contribution @SPGoding! Hopefully this will be released soon!

See just-the-docs/just-the-docs#578, just-the-docs/just-the-docs#857, just-the-docs/just-the-docs#876, and just-the-docs/just-the-docs#949.

The navigation should only display the external links once, after the links to pages that are not in collections. The test for PR just-the-docs#876 at https://just-the-docs.github.io/just-the-docs-tests/navigation/external-links fails with v0.4.0.rc3, and succeeds when the updated `nav.html` is added locally. The docs need updating to clarify how the interaction between the collections feature and the external links feature is resolved.

Fix external links and collections The navigation should only display the external links once, after the links to pages that are not in collections. The test for PR #876 at https://just-the-docs.github.io/just-the-docs-tests/navigation/external-links fails with v0.4.0.rc3, and succeeds when the updated `nav.html` is added locally. The docs need updating to clarify how the interaction between the collections feature and the external links feature is resolved.

Support external navigation links

dd6ba6b

mattxwang requested changes Jul 5, 2022

View reviewed changes

mattxwang added the pr: needs revision label Jul 5, 2022

SPGoding added 4 commits July 5, 2022 08:10

Merge remote-tracking branch 'upstream/main' into feature/nav-externa…

029ced0

…l-links

Align external link icon to the text

71781b1

Move external link icon to its own _includes file

0b46651

Add screenreader support for external link icon

c680efa

Remove duplicated documentation

9e18488

mattxwang removed pr: needs revision labels Jul 12, 2022

SPGoding mentioned this pull request Jul 13, 2022

Advance warning for links that open in a new tab #881

Open

mattxwang approved these changes Jul 13, 2022

View reviewed changes

mattxwang changed the base branch from main to v0.4.0 July 13, 2022 05:14

mattxwang merged commit 009a0f9 into just-the-docs:v0.4.0 Jul 13, 2022

SPGoding deleted the feature/nav-external-links branch July 13, 2022 05:41

pdmosses mentioned this pull request Oct 14, 2022

Fix duplicated external links in collections #1001

Merged

captn3m0 mentioned this pull request Jan 4, 2023

[dep] Upgrade theme to 0.5.4 endoflife-date/endoflife.date#2243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Support external navigation links #876

Support external navigation links #876

Uh oh!

SPGoding commented Jun 29, 2022

Uh oh!

mattxwang left a comment

Uh oh!

Uh oh!

mattxwang Jul 5, 2022

Uh oh!

SPGoding Jul 5, 2022

Uh oh!

mattxwang Jul 5, 2022

Uh oh!

SPGoding Jul 5, 2022 •

edited

Loading

Uh oh!

SPGoding commented Jul 5, 2022 •

edited

Loading

Uh oh!

mattxwang commented Jul 6, 2022

Uh oh!

mattxwang commented Jul 12, 2022

Uh oh!

SPGoding commented Jul 13, 2022

Uh oh!

mattxwang left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	{% unless node.hide_icon %}<svg viewBox="0 0 24 24"><use xlink:href="#svg-external-link"></use></svg>{% endunless %}
	{% unless node.hide_icon %}<svg aria-hidden="true" viewBox="0 0 24 24"><use xlink:href="#svg-external-link"></use></svg>{% endunless %}

Uh oh!

Support external navigation links #876

Support external navigation links #876

Uh oh!

Conversation

SPGoding commented Jun 29, 2022

Uh oh!

mattxwang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattxwang Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

SPGoding Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

mattxwang Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

SPGoding Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SPGoding commented Jul 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattxwang commented Jul 6, 2022

Uh oh!

mattxwang commented Jul 12, 2022

Uh oh!

SPGoding commented Jul 13, 2022

Uh oh!

mattxwang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SPGoding Jul 5, 2022 •

edited

Loading

SPGoding commented Jul 5, 2022 •

edited

Loading