Improve VTag component #3870

Stagge · 2024-03-04T08:36:23Z

Fixes

Fixes #3190 by @dhruvkb

Description

The VTag component added in #3137 has been improved in the following ways:

All props are forwarded to the inner VButton
All events are emitted from the inner VButton
The content is displayed with a slot for the content instead of a prop.
Accessible labels to the links have been added to clearly indicate that the link is a tag.
Tests for the VTag component has been added.

Checklist

My pull request has a descriptive title (not a vague title likeUpdate 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.
I ran the DAG documentation generator (if applicable).

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.

Closes #3. Co-authored-by: FelixSjogren <felixarvidsjogren@gmail.com>

FEAT - Make VTag pass all props to inner VButton. (#3)

Closes #2 Co-authored-by: Stagge <jonatan.stagge@gmail.com>

Feat/2 slot for the content (#2)

Added tests for Vtag, tests include: All props are sent to VButton VTag renders slot content Renders anchor tag. Co-authored-by: Stagge <jonatan.stagge@gmail.com>

Closes #4

- Added aria-label to indicate that that the link is a tag

obulat

This is a great, and implements everything that's required in the issue, @DD2480-Group-23!
However, I am not sure about the accessibility. Right now, if this is implemented as is, a screen reader loses all information about the tag (previously, they would hear something like "Link cat" for each tag, now they hear "Link Tag" for every tag). I will add accessibility recommendations as soon as I get the reply from the #accessibility channel.

frontend/src/components/VMediaInfo/VMediaTags.vue

obulat · 2024-03-06T12:48:26Z

frontend/src/components/VTag/VTag.vue

-    >{{ title }}</VButton
+    v-bind ="$props"
+    v-on="$listeners"
+    aria-label="Tag"


Adding the aria-label like this will cause the screen reader to say something like "Link tag, link tag, link tag" - which is confusing because the user does not understand what the tag name is. This should probably be a computed text, something like " tag", but I'm not sure.

I asked in the Make WordPress Slack #accessibility channel (requires registration for access), and will add a comment here when I get a reply.

We received a reply from the accessibility team.

Using aria-label to provide more context is not recommended. It helps blind screen reader users, but makes things less accessible for other users.

For example:
Sighted users who use screen readers as an aid: they will see visible text that is different from what the screen reader announces.
Speech recognition software users: they will see some visible text and try to use that to issue a voice command but the assistive technology takes into consideration the accessible name provided by the aria-label, which would be different.
Generally, it’s best to not alter the accessible name.

So, for this PR, we need to remove aria-label completely.

obulat · 2024-03-06T16:52:16Z

frontend/src/components/VTag/VTag.vue

-    >{{ title }}</VButton
+    v-bind ="$props"
+    v-on="$listeners"
+    aria-label="Tag"


We received a reply from the accessibility team.

Using aria-label to provide more context is not recommended. It helps blind screen reader users, but makes things less accessible for other users.

For example:
Sighted users who use screen readers as an aid: they will see visible text that is different from what the screen reader announces.
Speech recognition software users: they will see some visible text and try to use that to issue a voice command but the assistive technology takes into consideration the accessible name provided by the aria-label, which would be different.
Generally, it’s best to not alter the accessible name.

So, for this PR, we need to remove aria-label completely.

obulat · 2024-03-06T16:52:24Z

frontend/src/components/VTag/VTag.vue

-    >{{ title }}</VButton
+    v-bind ="$props"
+    v-on="$listeners"
+    aria-label="Tag"


Suggested change

aria-label="Tag"

obulat · 2024-03-06T16:52:56Z

frontend/src/components/VTag/VTag.vue

+    v-bind ="$props"
+    v-on="$listeners"
+    aria-label="Tag"
+    ><slot></slot></VButton


Suggested change

><slot></slot></VButton

><slot /></VButton

We usually use the self-closing tag if there's no content.

Stagge · 2024-03-07T20:42:52Z

Thanks for the review! :)

I have implemented your suggestions in e41f162

obulat

The CI is currently failing. To fix the lint step, you need to run just lint in the repository and commit the changes.

obulat · 2024-03-12T12:16:22Z

frontend/src/components/VTag/VTag.vue

@@ -4,8 +4,9 @@
    size="small"
    variant="filled-gray"
    class="label-bold"
-    :href="href"
-    >{{ title }}</VButton
+    v-bind ="$props"


props are automatically passed to the child component in Vue, but some attrs are not. We need to bind attrs here.

Suggested change

v-bind ="$props"

v-bind ="$attrs"

I am going to draft this PR until the changes are addressed (linting and binding $attrs instead of $props)

zackkrida · 2024-03-25T15:20:50Z

Hi @Stagge are you interested in resuming work on this PR? If you aren't able to we can have a maintainer work on it for you to get it across the finish line. Thank you!

Stagge · 2024-03-25T17:19:29Z

Hey! Sadly we won't have time to finish this, so feel free to let someone take over. Thanks!

obulat · 2024-03-26T05:05:16Z

frontend/src/components/VTag/VTag.vue

@@ -4,8 +4,9 @@
    size="small"
    variant="filled-gray"
    class="label-bold"
-    :href="href"
-    >{{ title }}</VButton
+    v-bind ="$props"


Suggested change

v-bind ="$props"

v-bind ="$attrs"

obulat · 2024-03-27T06:20:06Z

Hey! Sadly we won't have time to finish this, so feel free to let someone take over. Thanks!

Thank you for all of your work here, @Stagge ! I couldn't update this PR due to permission issues, so I created a new one with these changes in #3975

Stagge and others added 7 commits February 27, 2024 13:04

FEAT - Make VTag pass all props to inner VButton. (#3)

292b200

Closes #3. Co-authored-by: FelixSjogren <felixarvidsjogren@gmail.com>

Merge pull request #5 from DD2480-Group-23/feat/3-forward-props

7f3bb2d

FEAT - Make VTag pass all props to inner VButton. (#3)

FEAT - Add lot for the content instead of a prop (#2)

a4d5f8b

Closes #2 Co-authored-by: Stagge <jonatan.stagge@gmail.com>

Merge pull request #6 from DD2480-Group-23/feat/2-slot-for-the-content

32141ba

Feat/2 slot for the content (#2)

TEST - Add tests for VTag (#7)

763dc22

Added tests for Vtag, tests include: All props are sent to VButton VTag renders slot content Renders anchor tag. Co-authored-by: Stagge <jonatan.stagge@gmail.com>

FEAT - Ensure inner VButton emits and handles events in VTag #4

89aec3b

Closes #4

Added accessibility label (#10)

5509947

- Added aria-label to indicate that that the link is a tag

Stagge requested a review from a team as a code owner March 4, 2024 08:36

Stagge requested review from zackkrida and obulat March 4, 2024 08:36

zackkrida added the 🧱 stack: frontend Related to the Nuxt frontend label Mar 5, 2024

zackkrida requested review from dhruvkb and removed request for zackkrida March 5, 2024 17:46

obulat removed the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Mar 6, 2024

obulat reviewed Mar 6, 2024

View reviewed changes

obulat requested changes Mar 6, 2024

View reviewed changes

Improvements from review

e41f162

zackkrida requested a review from obulat March 7, 2024 21:00

This comment was marked as outdated.

Sign in to view

obulat requested changes Mar 12, 2024

View reviewed changes

obulat marked this pull request as draft March 12, 2024 15:27

obulat reviewed Mar 26, 2024

View reviewed changes

obulat mentioned this pull request Mar 27, 2024

VTag improvements #3975

Merged

8 tasks

obulat closed this Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve VTag component #3870

Improve VTag component #3870

Stagge commented Mar 4, 2024

obulat left a comment

obulat Mar 6, 2024

obulat Mar 6, 2024

obulat Mar 6, 2024

obulat Mar 6, 2024

obulat Mar 6, 2024

Stagge commented Mar 7, 2024

This comment was marked as outdated.

obulat left a comment

obulat Mar 12, 2024

obulat Mar 12, 2024

zackkrida commented Mar 25, 2024

Stagge commented Mar 25, 2024

obulat Mar 26, 2024

obulat commented Mar 27, 2024

Improve VTag component #3870

Improve VTag component #3870

Conversation

Stagge commented Mar 4, 2024

Fixes

Description

Checklist

Developer Certificate of Origin

obulat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stagge commented Mar 7, 2024

This comment was marked as outdated.

obulat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zackkrida commented Mar 25, 2024

Stagge commented Mar 25, 2024

Choose a reason for hiding this comment

obulat commented Mar 27, 2024