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

Me gravatar menu accessibility #11613

Merged
merged 10 commits into from
Apr 14, 2020

Conversation

develric
Copy link
Contributor

@develric develric commented Apr 8, 2020

This PR does the following:

  • Fixes Me menu with Gravatar accessibility #11599 : using the TooltipCompat for the actionLayout custom me menu
  • Tries to make the usage of MeGravatarLoader > constructGravatarUrl a bit more consistent
  • Fixes RTL support for me menu: before this PR trying to set an RTL language results in the menu to not being visualized (fix here ea97163)

To test

Me menu tooltip on long press

  • long press the me menu and check you get a tooltip with the Me word

Me menu talkback announce

  • Enable TalkBack and check the me menu is announced with Me word

RTL support

  • Set English language and check you have the Me menu with Gravatar on the right
  • Try to long press and tap on it to smoke test the functionality
  • Set an RTL language (like Hebrew) and check you have the Me menu with Gravatar on the left
  • Try to long press and tap on it to smoke test the functionality
Screenshot_1586339164 Screenshot_1586339213

NOTE

@osullivanchris while working on this PR I was thinking if it could make sense to change the alignment of the Me Menu; currently I did not change things in this PR respect to what we had here but was looking into making the me menu area wider (64dp see image below) so that it aligns on a 16dp end instead of 8dp. I think 48dp is the material design standard menu width tbh but right/end alignment (especially looking to the main FAB at the bottom) looks a bit strange maybe; let me know if there can be some better related guidelines, thanks 😊

Screenshot_1586341716 Screenshot_1586341016

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Sorry, something went wrong.

develric added 4 commits April 8, 2020 10:57

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 8, 2020

You can test the changes on this Pull Request by downloading the APK here.

Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

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

Checked the long-press and its working fine for me. Just noting that on my device i get a different UI if I long press on the FAB, compared to long press on the me menu link. The version on the 'me' icon is consistent with the UI I'm seeing for long press on other nav bar icons though, so I assume its the correct style there.

@osullivanchris while working on this PR I was thinking if it could make sense to change the alignment of the Me Menu; currently I did not change things in this PR respect to what we had here but was looking into making the me menu area wider (64dp see image below) so that it aligns on a 16dp end instead of 8dp. I think 48dp is the material design standard menu width tbh but right/end alignment (especially looking to the main FAB at the bottom) looks a bit strange maybe; let me know if there can be some better related guidelines, thanks 😊

This looks much better. Thanks for thinking about this / spotting it. You're definitely right, it looks better now. In my mocks I had 16 too but maybe I never specified it.

One additional note that I know vertically we have 12dp and 12dp. It would be better if we had 8 or 16dp. But the icon looks too small at 32dp and too big at 48dp so unfortunately we have these split boxes in your grid. I hope you're ok with it! I did consider it, but it just didn't work with a different size image.

@develric
Copy link
Contributor Author

develric commented Apr 8, 2020

Thanks for the follow up @osullivanchris 🙇‍♂️.

The version on the 'me' icon is consistent with the UI I'm seeing for long press on other nav bar icons though, so I assume its the correct style there.

Yep I confirm I looked for consistency with the other menus long press tooltip.

One additional note that I know vertically we have 12dp and 12dp. It would be better if we had 8 or 16dp. [...] I did consider it, but it just didn't work with a different size image.

Thanks for the clarification and I agree that the spec-ed 32dp seems the best fit dimension we can have there 👍.

Changed the me menu width to 64dp here c5184cc, reporting below the resulting image.

@develric develric marked this pull request as ready for review April 8, 2020 15:50
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Apr 8, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@develric
Copy link
Contributor Author

NOTE: I made a quick check with dark mode and AFAIU it seems fine; pinging @khaykov if you can have a look 🙇‍♂️. Some things I could have doubts are ic_user_white_24dp icon (feelColor set to @android:color/white) and me_action_layout.xml setting the layout background as ?android:attr/actionBarItemBackground (previous PR #11598 )

Screenshot_1586851302 Screenshot_1586851329
Screenshot_1586851528 Screenshot_1586851517

@develric develric requested a review from khaykov April 14, 2020 10:13
@khaykov khaykov self-assigned this Apr 14, 2020
Copy link
Member

@khaykov khaykov left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes @develric ! Code looks good to me, and works as expected :)
Regarding dark mode, I checked the layout, and since it looks the same in both dark and light modes I think it's good :)

@khaykov
Copy link
Member

khaykov commented Apr 14, 2020

I was about to merge it, but found one small issue with icon color - give me a moment to investigate.

@khaykov
Copy link
Member

khaykov commented Apr 14, 2020

nvm, it was from me modifying code to test a couple of different scenarios :)

@khaykov khaykov merged commit e2a2489 into develop Apr 14, 2020
@khaykov khaykov deleted the issue/11599-me-gravatar-menu-accessibility branch April 14, 2020 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Me menu with Gravatar accessibility
3 participants