Make VLink component that wraps around both external and internal links #879

obulat · 2022-02-16T09:45:49Z

Fixes

Description

This PR supercedes #468, based on @krysal 's comment about wrapping internal links, too.

Now, VLink is a wrapper component that should be used for all links. It determines whether the link is internal or external by checking if the href attr starts with /.

External links:

use a element.
add target='_blank' that allows the links to open in another tab, instead of causing errors in an iframe.
adds rel='noopener noreferrer' to prevent some referrer vulnerabilities in older browsers (target='_blank'` is enough for modern browsers)

Internal links:

use NuxtLink component.
wrap the href in app.localePath to ensure that the correct locale is used, and pass it as the to prop to NuxtLink.

This PR moves the warning for missing href from VButton to VLink, so VButton doesn't have to handle the lack of href property.

Testing Instructions

Run the app and test out internal links and external links. Also check that the links in the navbar (pages) work as expected. External links should always open in a new tab.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

# Conflicts: # src/pages/image/_id.vue

sarayourfriend

LGTM! Still need to test this locally but I'm still excited for this 🎉 The only blocker right now is the attrs destructuring... I wish there was an eslint rule to warn against that, I feel like 99% of the time it's probably not the right thing to do. Maybe that's wrong though.

src/components/VLink.vue

sarayourfriend · 2022-02-16T09:51:41Z

src/components/VLink.vue

+    const { app } = useContext()
+    const isInternal = computed(() => attrs.href?.startsWith('/'))
+    const linkComponent = computed(() => (isInternal.value ? 'NuxtLink' : 'a'))
+    const { href, ...otherProperties } = attrs


Destructuring these from attrs will make their values non-reactive in the computed for linkProperties below. To maintain reactivity they need to stay as properties on props or attrs.

test/unit/specs/components/v-link.spec.js

sarayourfriend · 2022-02-16T10:02:43Z

I noticed three more <a>'s in the app that could use this:

DownloadButton (It's still using the DropdownButton component which doesn't use VButton underneath)
FooterSection: link to 4v licenses
FooterSection: link to FontAwesome

src/components/VLink.vue

# Conflicts: # src/components/AudioDetails/VAudioDetails.vue # src/components/FooterSection.vue

Use VLink in DownloadButton

src/components/VLink.vue

sarayourfriend

The MediaTag changes seem unrelated. Was it a faulty merge perhaps? 🤔

src/components/VLink.vue

sarayourfriend · 2022-02-17T14:59:31Z

src/components/VLink.vue

-      { immediate: true }
+    let linkProperties = computed(() =>
+      isInternal.value
+        ? { to: app?.localePath(props.href) ?? props.href }


In what case is app null? 😱

When testing 😆 That was probably a lazy hack to ensure tests pass without mocking the nuxt context :)

Oh dear! I wish it was easier to set up a globally enabled mock of the nuxt stuff 🤔

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

obulat · 2022-02-17T16:09:31Z

The MediaTag changes seem unrelated. Was it a faulty merge perhaps? 🤔

Do you mean src/components/VMediaTag/meta/VMediaTag.stories.mdx changes, @sarayourfriend ?

This PR is kind of very small: adding one component, but it touches so many files!

krysal · 2022-02-17T16:24:58Z

Relabelling as "priority: medium" as the linked issue is actually fixed in the main branch and this is more of an improvement.

sarayourfriend · 2022-02-17T20:21:29Z

Oh sorry, I was looking specifically at the merge commit, not the full diff for the PR 🤦

Anyway, it looks like making props.href required breaks client-side navigation of the result page 🎉 I guess in the interest of not trying to fix too many things at once here we could just make the links wait until href is defined to render?

…nverse-frontend into add/bigger_vlink_component

obulat · 2022-02-18T03:40:15Z

Relabelling as "priority: medium" as the linked issue is actually fixed in the main branch and this is more of an improvement.

Thank you for relabelling, I was thinking about making it a priority: high, actually.
Just want to note that the issue was about all the external <a> links, not just the homepage licenses. Try clicking on 'Go to source', or links in the pages menu on search-staging. The links will be opened in the same tab, and with the iframe on w.org, this will break.

# Conflicts: # src/components/VMigrationNotice.vue

obulat · 2022-02-18T08:56:26Z

In the single media item detail pages we have some links that are created before the href values are available (while the image/audio is being fetched).

To make VLink component handle links which set href after loading the data, I set it use a span element if href is not available (is undefined, '', or '#'). So, VLink can actually behave incorrectly if you forget to add an href to it.

Another solution could be to use v-if for such links, but I think it would be more difficult to remember that.
I see a great advantage in using the VLink as in this PR:

we won't forget to set target="_blank" and stop a lot of errors for users who want to open an external link.
we won't forget to add the localePath to NuxtLinks.

What do you think, @WordPress/openverse-frontend ?

sarayourfriend

LGTM! Things no longer crash because of this (there are still other crashes but unrelated to this change 👍)

sarayourfriend · 2022-02-18T15:06:33Z

src/components/VLink.vue

@@ -21,19 +23,26 @@ export default defineComponent({
  props: {
    href: {
      type: String,
-      required: true,
-      validator: (v) => !['', '#'].includes(v),


nit: I think we can keep the validator, otherwise accidental long-lived invalid href values won't ever get caught.

sarayourfriend · 2022-02-18T15:21:03Z

@obulat a solution I've used in past projects to easily identify "bad links" is to add some kind of horrific inline-styles to them that make them obviously stand out. Something like style="background-color: hotpink; border: 2rem solid green;" (restricted to dev environments of course).

zackkrida

lgtm! I've clicked around pretty extensively and text searched for raw <a> tags, in the repo, and didn't find anything that shouldn't be there.

Question for @WordPress/openverse-frontend: Does anyone know of an eslint rule we could introduce to warn on raw <a> tag usage in our Vue templates?

sarayourfriend · 2022-02-18T20:22:39Z

@zackkrida The no-restricted-syntax rule can probably do it.

# Conflicts: # src/components/VMediaInfo/VCopyLicense.vue

krysal

@obulat You were right! I forgot about the untouched links on internal pages. It's great we can now be safe about external links with these changes 🚀

zackkrida · 2022-02-21T02:15:17Z

@obulat I made a small companion PR with an eslint rule to enforce VLink usage:

#916

obulat added 8 commits February 15, 2022 17:03

Add VLink component for external links

6921e3b

Add VLink component for external links

622acc0

Use VLink for Download button

4379fb8

Apply code review suggestions and add localePath to NuxtLinks

8433e64

Revert DownloadButton change

e8d70c6

Use VLink component as a wrapper for a and NuxtLink

7b069dd

Use VLink component as a wrapper for a and NuxtLink

66a604e

Merge branch 'main' into add/bigger_vlink_component

87416fc

# Conflicts: # src/pages/image/_id.vue

obulat added 🟥 priority: critical Must be addressed ASAP 🛠 goal: fix Bug fix 🕹 aspect: interface Concerns end-users' experience with the software labels Feb 16, 2022

obulat requested a review from a team as a code owner February 16, 2022 09:45

obulat self-assigned this Feb 16, 2022

obulat requested review from zackkrida and stacimc February 16, 2022 09:45

obulat mentioned this pull request Feb 16, 2022

Add VLink component for external links #871

Closed

7 tasks

sarayourfriend reviewed Feb 16, 2022

View reviewed changes

Stop destructuring attrs in order to not lose reactivity

fb30b4c

sarayourfriend reviewed Feb 16, 2022

View reviewed changes

src/components/VLink.vue Outdated Show resolved Hide resolved

obulat added 5 commits February 16, 2022 20:21

Merge branch 'main' into add/bigger_vlink_component

5a68ae6

# Conflicts: # src/components/AudioDetails/VAudioDetails.vue # src/components/FooterSection.vue

Use VLink for Download Button

673d8fb

Use VLink in DownloadButton

Fix homepage image link URL

7465072

Use href prop instead of href as an attr

4d7dfa0

VLink unit test improvements

fcd3257

sarayourfriend reviewed Feb 17, 2022

View reviewed changes

src/components/VLink.vue Outdated Show resolved Hide resolved

sarayourfriend reviewed Feb 17, 2022

View reviewed changes

Update src/components/VLink.vue

e8ed2ea

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Remove null check for app

231510c

krysal added 🟨 priority: medium Not blocking but should be addressed soon and removed 🟥 priority: critical Must be addressed ASAP labels Feb 17, 2022

Merge branch 'add/bigger_vlink_component' of github.com:WordPress/ope…

d52ed20

…nverse-frontend into add/bigger_vlink_component

obulat added 3 commits February 18, 2022 07:06

Merge branch 'main' into add/bigger_vlink_component

69345ad

Use span for links without href

482d7e1

Merge branch 'main' into add/bigger_vlink_component

d7ab14a

# Conflicts: # src/components/VMigrationNotice.vue

Add some validation

ebed7be

sarayourfriend approved these changes Feb 18, 2022

View reviewed changes

zackkrida approved these changes Feb 18, 2022

View reviewed changes

obulat added 2 commits February 20, 2022 08:34

Merge branch 'main' into add/bigger_vlink_component

1d7e2a7

# Conflicts: # src/components/VMediaInfo/VCopyLicense.vue

Check for app not being undefined in tests

7a4faff

krysal approved these changes Feb 21, 2022

View reviewed changes

obulat merged commit 69e096f into main Feb 21, 2022

obulat deleted the add/bigger_vlink_component branch February 21, 2022 02:24

obulat mentioned this pull request Feb 22, 2022

Open external links in parent frame #983

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make VLink component that wraps around both external and internal links #879

Make VLink component that wraps around both external and internal links #879

obulat commented Feb 16, 2022 •

edited

Loading

sarayourfriend left a comment

sarayourfriend Feb 16, 2022

sarayourfriend commented Feb 16, 2022

sarayourfriend left a comment

sarayourfriend Feb 17, 2022

obulat Feb 17, 2022

sarayourfriend Feb 17, 2022

obulat commented Feb 17, 2022

krysal commented Feb 17, 2022

sarayourfriend commented Feb 17, 2022

obulat commented Feb 18, 2022 •

edited

Loading

obulat commented Feb 18, 2022

sarayourfriend left a comment

sarayourfriend Feb 18, 2022

sarayourfriend commented Feb 18, 2022 •

edited

Loading

zackkrida left a comment •

edited

Loading

sarayourfriend commented Feb 18, 2022

krysal left a comment

zackkrida commented Feb 21, 2022

Make VLink component that wraps around both external and internal links #879

Make VLink component that wraps around both external and internal links #879

Conversation

obulat commented Feb 16, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 16, 2022

Choose a reason for hiding this comment

sarayourfriend commented Feb 16, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 17, 2022

Choose a reason for hiding this comment

obulat Feb 17, 2022

Choose a reason for hiding this comment

sarayourfriend Feb 17, 2022

Choose a reason for hiding this comment

obulat commented Feb 17, 2022

krysal commented Feb 17, 2022

sarayourfriend commented Feb 17, 2022

obulat commented Feb 18, 2022 • edited Loading

obulat commented Feb 18, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Feb 18, 2022

Choose a reason for hiding this comment

sarayourfriend commented Feb 18, 2022 • edited Loading

zackkrida left a comment • edited Loading

Choose a reason for hiding this comment

sarayourfriend commented Feb 18, 2022

krysal left a comment

Choose a reason for hiding this comment

zackkrida commented Feb 21, 2022

obulat commented Feb 16, 2022 •

edited

Loading

obulat commented Feb 18, 2022 •

edited

Loading

sarayourfriend commented Feb 18, 2022 •

edited

Loading

zackkrida left a comment •

edited

Loading