-
Notifications
You must be signed in to change notification settings - Fork 63
Add VLink component for external links #871
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 awesome! Love this kind of thing 🎉
Left a couple of non-blocking suggestions of things we can fix up if you want.
Additionally: in VButton we log a warning for a
's that are missing href
attribute. It'd be nice to transport that assurance to this component as well, just to catch accidental places where we bind it to a value and it turns up undefined, null, or as an empty string or something.
It might also be nice to throw a warning if we catch a non-external link being passed as those should all use NuxtLink
anyway, right?
Finally: some simple unit tests would be nice as well 🙂
None of that are blockers though 👍
src/pages/meta-search.vue
Outdated
@@ -9,9 +9,9 @@ | |||
|
|||
<i18n path="meta-search-page.intro" tag="p" class="mb-4"> | |||
<template #link> | |||
<a aria-label="sources" href="/sources">{{ | |||
<NuxtLink aria-label="sources" :to="localePath('/sources')">{{ |
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.
If we're already here, can we go ahead and translate the sources
string in the aria-label if you don't mind?
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.
Nice, looking at this we save several lines using a component. I wonder if it would be a good idea to make it more general and let it use an anchor or NuxtLink
when it's an internal or external link, as it is already done elsewhere, but I don't remember exactly where.
If not, don't you think we should rename it to ExternalLink
or something like that?
@krysal do you think we should have it auto-detect when a relative path vs external path is passed in? It could automatically call to |
Thank you for your great suggestions, @sarayourfriend and @krysal ! I decided to open a new PR #879 for a larger VLink component for both external and internal links, as Krystle suggested.
|
Fixes
Fixes #468 by @obulat
Description
This PR adds a simple
VLink
component that is a wrapper over ana
anchor with thetarget="_blank" rel="noopener noreferrer"
added to it, and replaces all externala
links in the components withVLink
.This ensures that the links can be opened from the iframe, and reduces the cognitive load of having to remember to add the link properties every time.
An interesting thing I learned:
target="_blank"
is not the same astarget="blank"
, with the latter opening all the links in a single new tab, and the former opening each new link in a new tab. I think we want the former behavior, but this is open for a discussion.Also, in the modern browsers,
rel="noopenver"
is automatically added to all the links withtarget="_blank"
, but it's nice to have it for older browsers.I did not initially replace the
DownloadButton
's<a>
. Trying out the DownloadButton on the production site, wordpress.org/openverse, with a sound from Freesound, I got a similar error screen as in the linked issue, so I used theVLink
component forDownloadButton
as well. But then I realized that the problem is caused by me not being signed in to Freesound, Jamendo audios can be downloaded from the deployed site. But I reverted the change anyway because the tests fail. If we decide to change it, we can do it in a new PR.Unrelated bug-fix: removes the bug-report state that was removed in a recent PR from the
feedback
page.Testing Instructions
Running the app, you should see all the external links working properly and opening in a new tab.
Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin