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

Fixes #2325: A11y + Highfi changes to audio player #3139

Merged
merged 15 commits into from
May 7, 2021
Merged

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Apr 28, 2021

Explanation

Fixes #2325

Mocks: https://xd.adobe.com/view/dfe8ccd4-39eb-414d-a0b2-b5674357f966-8abb/screen/b73cb76c-3593-4a30-944a-ab240f00d0df/specs/

  • As per the mocks the height of the audio player should be of 48dp. Now in previous implementation the height was more than 48dp because of incorrect calculations of height of the images.
  • This PR fixes the height of images by making changes to padding correctly. Also the bottom margin of root layout has been removed as it does not work across different layouts.
  • Also the gap between the play/pause and seekbar start is also reduced.

Note to reviewer

  • To test this I suggest using Galaxy Nexus emulator as it almost resembles the mocks.
  • If the below images look good to you than there is no need to check app for UI on device.
  • This screen passes all A11y checks mentioned in Changes in Views related to Audio Player [A11y] [BLOCKED] #2325
  • Talkback output has been added below.

Before vs. After vs. Mock

Before After Mock

Talkback output

audio_a11y.mp4

Espresso Test Result

Screenshot 2021-04-30 at 10 14 20 AM

Filed an issue for the above test failures because they are failing on develop too. Will try to to solve in separate PR.

Screenshot 2021-04-30 at 10 33 35 AM

There was one more test failing in AudioFragmentTest which is no longer needed therefore removed it in my last commit.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@rt4914 rt4914 requested a review from BenHenning as a code owner April 28, 2021 15:41
@rt4914 rt4914 changed the title Fixes #2325: Highfi changes to audio player Fixes #2325: A11y + Highfi changes to audio player Apr 28, 2021
@rt4914 rt4914 assigned rt4914 and BenHenning and unassigned rt4914 and BenHenning Apr 28, 2021
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Apr 28, 2021
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @rt4914! I think the solution looks good. Just have two additional questions besides my other comment from this review:

  1. Can you post an indication whether the affected tests still pass on Espresso?
  2. Is item 2 from Changes in Views related to Audio Player [A11y] [BLOCKED] #2325 not addressable/not planned to be addressed in this PR?

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Apr 29, 2021
@rt4914
Copy link
Contributor Author

rt4914 commented Apr 30, 2021

Thanks @rt4914! I think the solution looks good. Just have two additional questions besides my other comment from this review:

  1. Can you post an indication whether the affected tests still pass on Espresso?
  2. Is item 2 from Changes in Views related to Audio Player [A11y] [BLOCKED] #2325 not addressable/not planned to be addressed in this PR?
  1. I have added details in the description. Summary: Removed one test from AudioFragmentTest and filed Audio related tests failing on espresso #3145 as these tests are failing on develop.
  2. It has been solved in Fixes part of #2325: Changes in Cellular dialog[a11y] #2519 by Arjun.

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Apr 30, 2021
@rt4914
Copy link
Contributor Author

rt4914 commented Apr 30, 2021

Thanks @rt4914! I think the solution looks good. Just have two additional questions besides my other comment from this review:

  1. Can you post an indication whether the affected tests still pass on Espresso?
  2. Is item 2 from Changes in Views related to Audio Player [A11y] [BLOCKED] #2325 not addressable/not planned to be addressed in this PR?
  1. I have added details in the description. Summary: Removed one test from AudioFragmentTest and filed Audio related tests failing on espresso #3145 as these tests are failing on develop.
  2. It has been solved in Fixes part of #2325: Changes in Cellular dialog[a11y] #2519 by Arjun.

@BenHenning The failing tests for ExplorationActivityTest have been fixed in #3147

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @rt4914. Just 2 nits, otherwise PR LGTM.

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning May 1, 2021
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 May 4, 2021
@rt4914
Copy link
Contributor Author

rt4914 commented May 4, 2021

@BenHenning I am unable to understand why the bazel checks are failing.

@BenHenning
Copy link
Sponsor Member

BenHenning commented May 6, 2021

@rt4914 you probably already know this by now, but this is likely #3157.

@anandwana001 anandwana001 added the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 7, 2021
@rt4914 rt4914 removed the PR don't merge - NEEDS UPDATE Corresponds to PRs that need to be updated with the latest develop changes before they can be merged label May 7, 2021
@rt4914 rt4914 merged commit 87f28a1 into develop May 7, 2021
@rt4914 rt4914 deleted the highfi-audio-player branch May 7, 2021 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes in Views related to Audio Player [A11y] [BLOCKED]
3 participants