Skip to content

external link icon#59

Merged
rikedyp merged 2 commits into
mainfrom
54-external-links
Jun 2, 2026
Merged

external link icon#59
rikedyp merged 2 commits into
mainfrom
54-external-links

Conversation

@rikedyp
Copy link
Copy Markdown
Collaborator

@rikedyp rikedyp commented May 29, 2026

Addresses #54. Close only after related Dyalog/documentation#820 is completed

@rikedyp rikedyp requested a review from abrudz June 1, 2026 10:15
Comment thread javascripts/external-links.js Outdated
if (a.href.startsWith(window.location.origin)) return;
a.classList.add('external-link');
a.setAttribute('target', '_blank');
a.setAttribute('rel', 'noopener noreferrer');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

noreferrer prevents some forms of analytics, but is only really warranted for secure pages with secrets in the URL, i.e. not relevant here. It also implies noopener, and some older browser do not handle both being specified.

noopener is implied by target=_blank, so the whole thing can be removed.

Comment thread javascripts/external-links.js Outdated
if (a.protocol !== 'http:' && a.protocol !== 'https:') return;
if (a.href.startsWith(window.location.origin)) return;
a.classList.add('external-link');
a.setAttribute('target', '_blank');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Don't set target=_blank unless you want to annoy people. If people want to open the link in a new window, then their browser gives them the option, usually via Ctrl+click or similar. But if we set this, then they have no option to open the link in the current window.

Seriously, it is both bad for accessibility and doesn't achieve its goal; best practice is to not do this.

Comment thread javascripts/external-links.js Outdated
if (a.classList.contains('external-link')) return;
if (a.protocol !== 'http:' && a.protocol !== 'https:') return;
if (a.href.startsWith(window.location.origin)) return;
a.classList.add('external-link');
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why use awkward JS when simple CSS suffices‽ People might even have JS switched off.

Just make the selector be a:[href^="http"]:not([href^="https://docs.dyalog.com/"]:not([href^="https://dyalog.github.io/documentation/"])

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I thought about this and wanted to avoid it not showing up on local previews and having to document (where?) that external links show in the built site but not local previews. But maybe we can add also ^http://localhost. I'll try it.

@rikedyp rikedyp requested a review from abrudz June 2, 2026 14:12
@rikedyp rikedyp merged commit 5c28587 into main Jun 2, 2026
@rikedyp rikedyp deleted the 54-external-links branch June 2, 2026 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants