Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Remove span from VLink #1841

Merged
merged 4 commits into from
Oct 4, 2022
Merged

Remove span from VLink #1841

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9cd2fd8
Make VLink render only `<a>` or `nuxt-link`
obulat Sep 20, 2022
d666490
Simplify classes
obulat Sep 20, 2022
8f643b0
Temporarily skip a test
obulat Sep 20, 2022
40a14b8
Use `a` without `href` with `role=link` for (disabled) links without …
obulat Sep 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/components/VContentLink/VContentLink.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
<!-- We 'disable' the link when there are 0 results by removing the href and setting aria-disabled. -->
<VLink
:href="hasResults ? to : undefined"
role="link"
:aria-disabled="!hasResults"
class="flex w-full flex-col items-start overflow-hidden rounded-sm border border-dark-charcoal/20 bg-white py-4 ps-4 pe-12 md:flex-row md:items-center md:justify-between md:p-6"
:class="
hasResults
Expand Down
42 changes: 10 additions & 32 deletions src/components/VLink.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,21 @@
<!-- eslint-disable vue/no-restricted-syntax -->
<template>
<NuxtLink
v-if="linkComponent === 'NuxtLink'"
v-bind="$attrs"
v-if="isNuxtLink"
:class="{ 'inline-flex flex-row items-center gap-2': showExternalIcon }"
:to="linkTo"
v-on="$listeners"
@click.native="$emit('click', $event)"
>
<slot /><VIcon
v-if="showExternalIcon && !isInternal"
:icon-path="externalLinkIcon"
class="inline-block"
:size="4"
rtl-flip
/>
<slot />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized just now that a NuxtLink is only used for the internal links, so it cannot use the externalLinkIcon.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. How do we define an "external" link? When we've removed the iframe, will w.org links be "external"? If all links that don't use NuxtLink are external then we can remove the conditional showExternalIcon && !isInternal from VIcon. If it's only things that are not either (a) Openverse frontend links or (b) w.org links, then we could detect it and automatically add the icon without having to rely on the prop.

Probably an additional tightening up here and clarification around the iframe project and the idea of "externality". We typically concern ourselves with this (I think) due to differing privacy expectations on other websites. Given Openverse and WordPress.org follow the same privacy policies and are all controlled by WPF, we probably don't need the external icon for w.org links either. But again, something to clarify and probably something for another issue and to rope the w.org infrastructure folks in on.

</NuxtLink>
<a
v-else-if="linkComponent === 'a'"
v-bind="$attrs"
v-else
:href="href"
target="_blank"
rel="noopener noreferrer"
:role="href ? undefined : 'link'"
:aria-disabled="!href"
:class="{ 'inline-flex flex-row items-center gap-2': showExternalIcon }"
v-on="$listeners"
>
Expand All @@ -33,26 +27,13 @@
rtl-flip
/>
</a>
<span
v-else
v-bind="$attrs"
:class="{ 'inline-flex flex-row items-center gap-2': showExternalIcon }"
>
<slot /><VIcon
v-if="showExternalIcon && !isInternal"
:icon-path="externalLinkIcon"
class="inline-block"
:size="4"
rtl-flip
/>
Comment on lines -41 to -47
Copy link
Member

Choose a reason for hiding this comment

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

Is the removal of <VIcon/> accidental or intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the Icon inside the span I'd removed, and NuxtLink still have it.

</span>
</template>

<script lang="ts">
/**
* This is a wrapper component for all links. If a link is dynamically generated and doesn't have
* an `href` prop (as the links for detail pages when the image detail hasn't loaded yet),
* it is rendered as a `span`.
* This is a wrapper component for all links. If `href` prop is undefined,
* the link will be rendered as a disabled: an `<a>` element without `href`
* attribute and with `role="link"` and `aria-disabled="true"` attributes.
* Links with `href` starting with `/` are treated as internal links.
*
* Internal links use `NuxtLink` component with `to` attribute set to `localePath(href)`
Expand All @@ -67,7 +48,6 @@ import externalLinkIcon from '~/assets/icons/external-link.svg'
export default defineComponent({
name: 'VLink',
components: { VIcon },
inheritAttrs: false,
props: {
href: {
type: String,
Expand Down Expand Up @@ -96,9 +76,7 @@ export default defineComponent({
const isInternal = computed(
() => hasHref.value && props.href?.startsWith('/')
)
const linkComponent = computed(() =>
hasHref.value ? (isInternal.value ? 'NuxtLink' : 'a') : 'span'
)
const isNuxtLink = computed(() => hasHref.value && isInternal.value)

let linkTo = computed(() =>
checkHref(props) && isInternal.value
Expand All @@ -108,7 +86,7 @@ export default defineComponent({

return {
linkTo,
linkComponent,
isNuxtLink,
isInternal,
externalLinkIcon,
}
Expand Down