fix: accessibility issue with feeds comment section #14958

pramodrhegde · 2021-10-05T17:35:33Z

What type of PR is this? (check all applicable)

Description

Improved the accessibility of the feeds comment section by adding a more meaningful name to the comment button element. fixes: #14881

Comments section name before the fix :

On mobile the accessible name for this link is either:

Completely blank (if no comments exist)
Just the number (e.g. a link with name '1')
On desktop the accessible names are:

Add comment (if no comments exist)
"1 comment" / "X comments" (if comments do exist)

Comments section name after the fix :

Both on mobile and the desktop:

for both cases (if no comments exist)(if no comments exist) below will be the name for the comments section on a feed:
Comments for article ${articleTitle}. ${count} comment(s) so far

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.

UI accessibility concerns?

With this fix accessibility of the site has been improved in the comments section on a feed. now the name of the comment section is more meaningful and conveys a unique message based on the article title and number of comments on the article so far. attached are the screenshots below for mobile and desktop versions:

For Mobile:

For Desktop:

Verified these changes by running a screen reader in local.

Added/updated tests?

Yes
No, and this is why: please replace this line with details on why tests
have not been included
I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

I've updated the Developer Docs or
Storybook (for Crayons components)
This PR changes the Forem platform and our documentation needs to be
updated. I have filled out the
Changes Requested
issue template so Community Success can help update the Admin Docs
appropriately.
I've updated the README or added inline documentation
I've added an entry to
CHANGELOG.md
I will share this change in a Changelog
or in a forem.dev post
I will share this change internally with the appropriate teams
I'm not sure how best to communicate this change and need help
This change does not need to be communicated, and this is why not: please
replace this line with details on why this change doesn't need to be
shared

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

CLAassistant · 2021-10-05T17:35:37Z

All committers have signed the CLA.

github-actions · 2021-10-05T17:35:47Z

Thank you for opening this PR! We appreciate you!

For all pull requests coming from third-party forks we will need to
review the PR before we can process it through our CI pipelines.

A Forem Team member will review this contribution and get back to
you as soon as possible!

aitchiss

This is looking good! The only issue is that the Preact component is only used for the logged in home feed, so the new changes aren't currently impacting the logged out feed, paged in articles or search results for posts.

I think if you apply your same changes to _single_story.html.erb and buildArticleHTML.js, we should be good to go 🙂

aitchiss · 2021-10-06T13:22:03Z

app/javascript/articles/components/CommentsCount.jsx

@@ -15,6 +14,7 @@ export const CommentsCount = ({ count, articlePath }) => {
      <path d="M10.5 5h3a6 6 0 110 12v2.625c-3.75-1.5-9-3.75-9-8.625a6 6 0 016-6zM12 15.5h1.5a4.501 4.501 0 001.722-8.657A4.5 4.5 0 0013.5 6.5h-3A4.5 4.5 0 006 11c0 2.707 1.846 4.475 6 6.36V15.5z" />
    </svg>
  );
+  const commentsAriaLabelText = `Comments for article ${articleTitle}. ${count} comment(s) so far`;


suggestion (not blocking) - maybe consider something shorter, e.g. Comments for article ${articleTitle} (${count})

Just thinking for screen reader users who need to listen to everything being read aloud, it can be quite verbose, especially if the post has a long title 🙂

Make sense @aitchiss will do the changes. thank you!

pramodrhegde · 2021-10-06T15:06:49Z

This is looking good! The only issue is that the Preact component is only used for the logged in home feed, so the new changes aren't currently impacting the logged out feed, paged in articles or search results for posts.

I think if you apply your same changes to _single_story.html.erb and buildArticleHTML.js, we should be good to go 🙂

Added screenshots below for the refactored changes:

Logged out scenario

Search results

aitchiss

This is looking great - thanks for applying those changes to the other files! Something that occurred to me later was that we use "post" terminology in most places now, rather than "article", so I've left a few suggestions to make sure we're using the same terminology in the aria-label too (sorry - didn't think about that earlier!)

app/assets/javascripts/utilities/buildArticleHTML.js

app/javascript/articles/components/CommentsCount.jsx

app/views/articles/_single_story.html.erb

pramodrhegde · 2021-10-07T13:04:28Z

This is looking great - thanks for applying those changes to the other files! Something that occurred to me later was that we use "post" terminology in most places now, rather than "article", so I've left a few suggestions to make sure we're using the same terminology in the aria-label too (sorry - didn't think about that earlier!)

No problem, Updated PR with the changes. please take a look.

rhymes

LGTM! Thanks a lot @pramodrhegde

aitchiss

Looks great! Thanks for making that final change - I'm excited to get this merged ✨

The failed build doesn't seem to be anything related to your changes, so I've hit restart on it. Hopefully it'll pass shortly and then this is good to go 🚀

aitchiss · 2021-10-07T15:29:52Z

Hey @pramodrhegde looks like we still need you to sign the Contributor License Agreement - once you've been able to do that, we'll be able to merge 🚀

pramodrhegde · 2021-10-07T16:01:18Z

Signed! thank you @aitchiss and @rhymes for helping with the review ❤️

fix: accessibility issue with feeds comment section

d586ff6

pramodrhegde requested review from a team, Ridhwana and aitchiss and removed request for a team October 5, 2021 17:35

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Oct 5, 2021

aitchiss reviewed Oct 6, 2021

View reviewed changes

pramodrhegde added 2 commits October 6, 2021 20:28

fix: comments section name for logged out scenario and search results

700a891

fix: review changes for logged in scenario

f89bd82

aitchiss reviewed Oct 7, 2021

View reviewed changes

fix: updated comments section aria label

3708475

rhymes approved these changes Oct 7, 2021

View reviewed changes

aitchiss approved these changes Oct 7, 2021

View reviewed changes

pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Oct 7, 2021

aitchiss merged commit ce4ba84 into forem:main Oct 7, 2021

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Oct 7, 2021

fix: accessibility issue with feeds comment section #14958

fix: accessibility issue with feeds comment section #14958

pramodrhegde commented Oct 5, 2021 •

edited

CLAassistant commented Oct 5, 2021 •

edited

github-actions bot commented Oct 5, 2021

aitchiss left a comment

aitchiss Oct 6, 2021

pramodrhegde Oct 6, 2021

pramodrhegde commented Oct 6, 2021

aitchiss left a comment

pramodrhegde commented Oct 7, 2021

rhymes left a comment

aitchiss left a comment

aitchiss commented Oct 7, 2021

pramodrhegde commented Oct 7, 2021 •

edited

fix: accessibility issue with feeds comment section #14958

fix: accessibility issue with feeds comment section #14958

Conversation

pramodrhegde commented Oct 5, 2021 • edited

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

UI accessibility concerns?

Added/updated tests?

[Forem core team only] How will this change be communicated?

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

CLAassistant commented Oct 5, 2021 • edited

github-actions bot commented Oct 5, 2021

aitchiss left a comment

Choose a reason for hiding this comment

aitchiss Oct 6, 2021

Choose a reason for hiding this comment

pramodrhegde Oct 6, 2021

Choose a reason for hiding this comment

pramodrhegde commented Oct 6, 2021

aitchiss left a comment

Choose a reason for hiding this comment

pramodrhegde commented Oct 7, 2021

rhymes left a comment

Choose a reason for hiding this comment

aitchiss left a comment

Choose a reason for hiding this comment

aitchiss commented Oct 7, 2021

pramodrhegde commented Oct 7, 2021 • edited

pramodrhegde commented Oct 5, 2021 •

edited

CLAassistant commented Oct 5, 2021 •

edited

pramodrhegde commented Oct 7, 2021 •

edited