Remove span from VLink #1841
Remove span from VLink #1841
Conversation
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1841 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
✨ goal: improvement
| <slot /><VIcon | ||
| v-if="showExternalIcon && !isInternal" | ||
| :icon-path="externalLinkIcon" | ||
| class="inline-block" | ||
| :size="4" | ||
| rtl-flip | ||
| /> |
There was a problem hiding this comment.
Is the removal of <VIcon/> accidental or intentional?
There was a problem hiding this comment.
Only the Icon inside the span I'd removed, and NuxtLink still have it.
|
Size Change: -3.68 kB (0%) Total Size: 811 kB
ℹ️ View Unchanged
|
|
My understanding was that |
Oh, this is interesting, thank you for links, Zack! I tried tabbing to the disabled content links on staging - and couldn't. Those links are skipped when using the Tab key. However, with VO, if you use Ctrl Shift arrow key - you do get to the disabled link! |
|
@obulat do you think we should remove the |
obulat
force-pushed
the
remove_span_from_vlink
branch
from
7747d0a
to
40a14b8
Compare
| :size="4" | ||
| rtl-flip | ||
| /> | ||
| <slot /> |
There was a problem hiding this comment.
I realized just now that a NuxtLink is only used for the internal links, so it cannot use the externalLinkIcon.
There was a problem hiding this comment.
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.
Based on the articles you've linked, I made the changes to render |
obulat
added
🟧 priority: high
obulat
added
🟨 priority: medium
|
@sarayourfriend, I would really appreciate your opinion on this approach for disabling links, and not having |
|
If you're asking from an a11y perspective, I don't really know enough to comment, but I can speculate based on what I know an my small experience using screen readers. In my opinion, the behaviour you're seeing with VO is fine, correct, consistent, and expected. Tab is only used to move between actionable items (buttons, form elements, links) so it naturally skips over disabled ones. The VO text navigation on the other hand is used to read each element of the page one by one. Tab navigation is probably more likely used by a sighted keyboard user who will have the visual context to see that a link is disabled. Maybe some style elements would need to show this if it's anything beyond a loading state so that all sighted users who are not relying on a screen reader that will relay the aria attribute context to them will understand why something that appears to be a link is not clickable. These are all problems that already exist if we use a span fallback, so I don't think they need to be solved or debated too much here. I haven't tested the code here so I won't stick an approval on it, but as long as other folks have tested and find it functional, I don't see any a11y reason not to merge this. There is no regression from the previous behaviour and in fact screen readers will now have more context than they did before with the |
Fixes
Fixes #1840 by @obulat
Description
This PR removes
spanas the fallback element that was rendered byVLinkif thehrefprop wasn't provided.It also adds checks for
hrefin all components that use a dynamichrefprop, and adds a wrapper to theVContentLinkto make the disabled component render a div instead of analink without anhref.Testing Instructions
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin