-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
There was a problem hiding this 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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icon found
@randimays I think there's a missing heading/title on the last link on the form detail pages.
|
f1a2c3e
to
beba784
Compare
There was a problem hiding this 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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icon found
beba784
to
1a6fe99
Compare
There was a problem hiding this 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
@laflannery I believe this problem has been fixed; thanks for catching it! It's ready for your review |
Looks good, thanks @randimays! |
There was a problem hiding this 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! 👏🏻
Summary
Finish up Find a Form link conversion to web components.
Also includes accessibility adjustments: remove
role="presentation"
on icons that already havearia-hidden="true"
.Related issue(s)
Testing done
Tested locally at
/find-forms
.Screenshots