-
Notifications
You must be signed in to change notification settings - Fork 148
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
mm-241 - adds aria labels to right hand side elements #462
Conversation
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. |
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
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 👍
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.
Should the PR/Issue title also have a label?
And what about the milestone icon? |
@hanzei is this ready to merge? Looks like it has two approvals. We can create follow-up tickets for the additional labels and icons? |
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.
Thanks @maisnamrajusingh, I have a request to reformat some JSX indenting. Also, I agree with @hanzei that we should add the aria-labels to the PR title and milestones, and I would like to add them to the PR/issue labels as well.
<span | ||
title={'Success'} | ||
style={{...style.icon, ...style.iconSucess}} | ||
><TickIcon/></span> |
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.
Can we format the JSX for this block and the two blocks below like this?
<span | |
title={'Success'} | |
style={{...style.icon, ...style.iconSucess}} | |
><TickIcon/></span> | |
<span | |
title={'Success'} | |
style={{...style.icon, ...style.iconSucess}} | |
> | |
<TickIcon/> | |
</span> |
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.
@maisnamrajusingh By "and the two blocks below" I meant to apply this change to the two blocks below this block as well. Can you please apply the change to the other two span
blocks?
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.
sure, done.
Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>
Codecov Report
@@ 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. |
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.
Blocking on by questions above: #462 (review)
@hanzei I have assigned the changes to @sanjaydemansol He will take a look into it |
I'm also blocking on #462 (comment) |
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
I guess it should have one, @sanjaydemansol can you add it. |
Please elaborate, what should be the content of label? |
Since there is already review and dicussion history on this PR, I think it makes sense to finish this one that's in progress |
@@ -165,6 +168,7 @@ function GithubItems(props: GithubItemsProps) { | |||
if (item.milestone) { | |||
milestone = ( | |||
<span | |||
title={'Milestone'} |
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 like we should have the milestone's name here, based on the discussion in this thread
Co-authored-by: Michael Kochell <6913320+mickmister@users.noreply.github.com>
status = (<span style={{...style.icon, ...style.iconSucess}}><TickIcon/></span>); | ||
status = ( | ||
<span | ||
title={'Success'} |
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.
I think "Checks Passing" would be more appropriate, but not a required change
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
/update-branch |
We don't have permissions to update this PR, please contact the submitter to apply the update. |
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.
Tested and passed
- For the area in scope here:
- All title spans have aria labels
- Other spans and divs seem to all have aria label
- All svg images have aria labels
- No regressions found
note The initial issue makes mention that some hover effects are missing. While all .svg now have a label this did not solve the problem of missing hover effects on some elements. However, if this is problematic we should open a new issue with specifically where we want text to show on hover. Therefore I believe we should accept this PR as is.
LGTM!
Summary
Hovering over the elements on the right hand side should display the labels for those items.
Ticket Link
#241