Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VACMS-17188 Remaining Find Forms links fixes and accessibility fixes #1960

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

randimays
Copy link
Contributor

Summary

Finish up Find a Form link conversion to web components.

Also includes accessibility adjustments: remove role="presentation" on icons that already have aria-hidden="true".

Related issue(s)

Testing done

Tested locally at /find-forms.

Screenshots

Screenshot 2024-03-12 at 4 41 45 PM Screenshot 2024-03-12 at 4 41 37 PM Screenshot 2024-03-12 at 4 41 32 PM Screenshot 2024-03-12 at 4 41 27 PM

@randimays randimays requested review from a team as code owners March 13, 2024 14:47
Copy link
Collaborator

@va-vfs-bot va-vfs-bot left a comment

Choose a reason for hiding this comment

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

Icon found

Icons can be decorative, but sometimes they are used to convey meaning. If there are any semantics associated with an icon, those semantics should also be conveyed to a screen reader.

What you can do

Review the markup and see if the icon provides information that isn't represented textually, or wait for a VSP review.

@@ -9,7 +9,7 @@
{% if forloop.index <= pageSize %}
<li class="vads-u-margin-y--2">
<a class="vads-u-font-weight--bold vads-u-text-decoration--none" href="{{ audienceTag.entityUrl.path }}/">
{{ audienceTag.name }} <i role="presentation" aria-hidden="true" class="fa fa-chevron-right vads-u-margin-left--1"></i>
{{ audienceTag.name }} <i aria-hidden="true" class="fa fa-chevron-right vads-u-margin-left--1"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

@@ -30,7 +30,7 @@
<li class="vads-u-margin-y--2">
<a class="vads-u-font-weight--bold vads-u-text-decoration--none" href="{{ audienceTag.path.alias }}/">
{{ audienceTag.name }}
<i role="presentation" aria-hidden="true" class="fa fa-chevron-right vads-u-margin-left--1"> </i>
<i aria-hidden="true" class="fa fa-chevron-right vads-u-margin-left--1"> </i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

@@ -23,7 +23,7 @@
{% for link in searchLinks %}
{% if link.url.path and link.label %}
<li>
<i role="presentation" class="fas fa-arrow-right vads-u-color--link-default vads-u-margin-right--1"></i>
<i aria-hidden="true" class="fas fa-arrow-right vads-u-color--link-default vads-u-margin-right--1"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

@@ -14,20 +14,20 @@
href="{{ entityUrl.path }}"
id="add-to-calendar-link"
>
<i class="va-c-social-icon fas fa-calendar-check vads-u-margin-right--0p5" aria-hidden="true" role="presentation"></i>
<i class="va-c-social-icon fas fa-calendar-check vads-u-margin-right--0p5" aria-hidden="true"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

Add to Calendar
</a>
</li>

<li class="vads-u-margin-bottom--2p5">
<a class="va-js-share-link" href="https://www.facebook.com/sharer/sharer.php?href={{ hostUrl }}{{ entityUrl.path }}">
<i aria-hidden="true" class="va-c-social-icon fab fa-facebook vads-u-margin-right--0p5" role="presentation"></i>Share on Facebook
<i aria-hidden="true" class="va-c-social-icon fab fa-facebook vads-u-margin-right--0p5"></i>Share on Facebook
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

</a>
</li>

<li>
<a class="va-js-share-link" href="https://twitter.com/intent/tweet?text={{ title }}&url={{ hostUrl }}{{ entityUrl.path }}">
<i aria-hidden="true" class="va-c-social-icon fab fa-twitter vads-u-margin-right--0p5" role="presentation"></i>Share on Twitter
<i aria-hidden="true" class="va-c-social-icon fab fa-twitter vads-u-margin-right--0p5"></i>Share on Twitter
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

<va-link active class="vads-u-text-decoration--none" href="{{ vaParagraph.entity.fieldLink.url.path }}" text="{{ vaParagraph.entity.fieldLink.title }}">
<i class="fa fa-chevron-right vads-u-padding-left--0p5 vads-u-font-size--sm" aria-hidden="true" role="presentation"></i>
<va-link active href="{{ vaParagraph.entity.fieldLink.url.path }}" text="{{ vaParagraph.entity.fieldLink.title }}">
<i class="fa fa-chevron-right vads-u-padding-left--0p5 vads-u-font-size--sm" aria-hidden="true"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

@va-vfs-bot va-vfs-bot temporarily deployed to master/main/17188-find-forms-links March 13, 2024 15:14 Inactive
@laflannery
Copy link

@randimays I think there's a missing heading/title on the last link on the form detail pages.

@randimays randimays force-pushed the 17188-find-forms-links branch from f1a2c3e to beba784 Compare March 13, 2024 17:26
Copy link
Collaborator

@va-vfs-bot va-vfs-bot left a comment

Choose a reason for hiding this comment

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

Icon found

Icons can be decorative, but sometimes they are used to convey meaning. If there are any semantics associated with an icon, those semantics should also be conveyed to a screen reader.

What you can do

Review the markup and see if the icon provides information that isn't represented textually, or wait for a VSP review.

@@ -23,7 +23,7 @@
{% for link in searchLinks %}
{% if link.url.path and link.label %}
<li>
<i role="presentation" class="fas fa-arrow-right vads-u-color--link-default vads-u-margin-right--1"></i>
<i role="presentation" aria-hidden="true" class="fas fa-arrow-right vads-u-color--link-default vads-u-margin-right--1"></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

icon found

Copy link
Contributor

@chriskim2311 chriskim2311 left a comment

Choose a reason for hiding this comment

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

LGTM! Reviewed in review instance

@randimays
Copy link
Contributor Author

@laflannery I believe this problem has been fixed; thanks for catching it! It's ready for your review

@laflannery
Copy link

Looks good, thanks @randimays!

Copy link
Contributor

@jamigibbs jamigibbs left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for using the web components! 👏🏻

@randimays randimays merged commit fa89b00 into main Mar 14, 2024
22 checks passed
@randimays randimays deleted the 17188-find-forms-links branch March 14, 2024 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants