mm-241 - adds aria labels to right hand side elements

[maisnamrajusingh]

Summary

Hovering over the elements on the right hand side should display the labels for those items.

Ticket Link

https://github.com/mattermost/mattermost-plugin-github/issues/241

[mattermod]

Hello @maisnamrajusingh,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

[hanzei]

And what about the milestone icon?

[jasonblais]

@hanzei is this ready to merge? Looks like it has two approvals. We can create follow-up tickets for the additional labels and icons?

[codecov-commenter]

Codecov Report

Merging #462 (e8a192c) into master (f585550) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #462   +/-   ##
=======================================
  Coverage   15.51%   15.51%           
=======================================
  Files          15       15           
  Lines        4151     4151           
=======================================
  Hits          644      644           
  Misses       3465     3465           
  Partials       42       42           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[maisnamrajusingh]

@hanzei I have assigned the changes to @sanjaydemansol He will take a look into it

[mickmister]

I'm also blocking on https://github.com/mattermost/mattermost-plugin-github/pull/462#discussion_r685672062

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

[maisnamraju]

Should the PR/Issue title also have a label?

I guess it should have one, @sanjaydemansol can you add it.

[sanjaydemansol]

Should the PR/Issue title also have a label?

Please elaborate, what should be the content of label?

[sanjaydemansol]

Did do add a label for the milestone icon?

What should be the content of label? "milestone" ?

[hanzei]

The milestone name should also be part of the label.

[sanjaydemansol]

The milestone name should also be part of the label.

Used word "Milestone", 2/5..its more helpful to know what 1.2.3 actually is. Let me know If any changes needed. @hanzei thanks.

[hanzei]

@maisnamrajusingh Did you noticed by comment at https://github.com/mattermost/mattermost-plugin-github/pull/462#pullrequestreview-803526431?

[sanjaydemansol]

@maisnamrajusingh Did you noticed by comment at #462 (review)?

@hanzei here is suggestion from author https://github.com/mattermost/mattermost-plugin-github/issues/241#issuecomment-885384763 for similar request https://github.com/mattermost/mattermost-plugin-github/issues/241#issuecomment-884797740. I guess https://chrome.google.com/webstore/detail/screen-reader/kgejglhpjiefppelpmljglcjbhoiplfn?hl=en is not applicable in our case, since this PR is to make Icons user friendly instead of adding Accessibility.

[mickmister]

@sanjaydemansol @hanzei The original intention of the ticket was to add accessibility, though I got confused and forgot that the browser's native tooltip from the title attribute also aids in accessbility.

I propose we introduce the aria-labels as originally planned, as well as using the tooltip functionality that's used throughout the rest of the application, using the OverlayTrigger and Tooltip components from react-bootstrap:

https://github.com/mattermost/mattermost-webapp/blob/242a18c68908bbcfee0effd5bc6804d711670c4a/components/channel_header/components/header_icon_wrapper.tsx#L134

[sanjaydemansol]

@sanjaydemansol @hanzei The original intention of the ticket was to add accessibility, though I got confused and forgot that the browser's native tooltip from the title attribute also aids in accessbility.

I propose we introduce the aria-labels as originally planned, as well as using the tooltip functionality that's used throughout the rest of the application, using the OverlayTrigger and Tooltip components from react-bootstrap:

https://github.com/mattermost/mattermost-webapp/blob/242a18c68908bbcfee0effd5bc6804d711670c4a/components/channel_header/components/header_icon_wrapper.tsx#L134

@mickmister thank you for the clarity. Will make changes to the PR.

[sanjaydemansol]

@mickmister @hanzei, is this what we are looking for? (latest commit). Thanks.

[maisnamrajusingh]

@hanzei I'm taking over this one. Just confirming if only the comment below is the remaining fix needed on this one ?

When testing with https://chrome.google.com/webstore/detail/screen-reader/kgejglhpjiefppelpmljglcjbhoiplfn?hl=en the following elements were not read out:

• CI check
• Labels
• Milestone
• Number of reviews
• Time when the PR/issue was opened

Could you please check why they are not read out?

[hanzei]

@maisnamrajusingh That is indeed the case.

[mickmister]

/update-branch

[mattermod]

We don't have permissions to update this PR, please contact the submitter to apply the update.

[mickmister]

It seems there are a lot of unrelated changes in GitHub's diff view. This is likely because this branch is behind master and got into a weird state. @maisnamrajusingh Can you please update the branch with master?

Also, when you submitted this PR, did you check the box to allow maintainers to update the PR? Based on the message above, it seems that box may not have been checked.

[mickmister]

@mickmister this was an original pr by @sanjaydemansol I am not sure if it was ticked or not. Should I just send a new PR in place of this ?

Since there is already review and dicussion history on this PR, I think it makes sense to finish this one that's in progress

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity. A core team member will check in on the status of the PR to help with questions. Thank you for your contribution!

[hanzei]

/update-branch

[mattermost-build]

We don't have permissions to update this PR, please contact the submitter to apply the update.