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

Fix part of #3245 [A11y] Enabling AccessibilityChecks for QuestionPlayerActivityTest #4173

Merged
merged 3 commits into from
Feb 22, 2022

Conversation

Rohit1173
Copy link
Contributor

@Rohit1173 Rohit1173 commented Feb 11, 2022

Explanation

Fix #3927 and Fix part of #3245 :Enable AccessibiltyChecks

Removed the @DisableAccessibilityChecks Annotation from the QuestionPlayerActivityTest
There are 2 tests with @DisableAccessibilityChecks Annotation in QuestionPlayerActivityTest
fun testQuestionPlayer_terminalState_recyclerViewItemCount_countIsTwo() and
fun testQuestionPlayer_terminalState_recyclerView_contentItem_isNotEmpty()
and these were the screenshots after successfully running them

Device : Pixel 3a API 28
A11y-1
A11y-2

So after Fixing the minheight and minwidth of feedback_text_view in feedback_item.xml to 48dp each and running again these were the results.

Device : Pixel 3a API 28
A11y -1 after
A11y-2 after

After that I have tested the above tests for tablets, these were the screenshots after successfully running them

Device : Pixel C API 28
A11y -1 tab before
A11y-2 tab before

So to fix these failing tests, I have set the minheight and minwidth of content_text_view in content_item.xml to 48dp each

After making these changes and running again, these were the results

Device : Pixel C API 28
A11y-1 tab after
A11y-2 tab after

Device : Pixel 3a API 28
A11y-1 mobile last
A11y-2 mobile last

UI CHANGES

BEFORE AFTER
mobile_before mobile after
mobile land before mobile land after
tablet before tablet after
tab land before tab land after

Tests in StateFragmentTest.kt before the changes have been made (Device : Pixel 3A API 28)
tests before state

Tests in StateFragmentTest.kt after the changes have been made (Device : Pixel 3A API 28)
after tests state

UI CHANGES

Before After
state before mobile state after mob
state before land mob after state mob land
state before tab after state tab
state  before tab land tab after state land

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@Rohit1173
Copy link
Contributor Author

@rt4914 I have got an error in tablet (Pixel-C API 28) to change the size of content_text_view but after changing it the tests were passed but the UI had changed and the content_item was shrunk. Could you please guide on me how to solve this

@rt4914
Copy link
Contributor

rt4914 commented Feb 14, 2022

@rt4914 I have got an error in tablet (Pixel-C API 28) to change the size of content_text_view but after changing it the tests were passed but the UI had changed and the content_item was shrunk. Could you please guide on me how to solve this

  1. I think we dont' need to worry about the width. Just mentioned minWidth=48dp should be fine.
  2. Also for height also use minWidth instead of layout_height.
  3. If there is any extra margin around the TextView you can shift that to padding of TextView and that way the height should automatically get fixed (upto certain extent and not fully).
  4. As the changes in this PR deal with UI change please create before/after screenshot in mobile-portrait, landscape and tablet-portrait, landscape for Question Player as well as Exploration Player. Also as we are dealing with min-height based solution suggest picking an example for screenshot where the feedback item contains only 1 line of text so that we can correctly see the changes in before/after screenshots

@rt4914
Copy link
Contributor

rt4914 commented Feb 14, 2022

Also if you need reply on anything please Assign me in Assignees section so that I know I have check the PR.

@Rohit1173
Copy link
Contributor Author

@rt4914 I have got an error in tablet (Pixel-C API 28) to change the size of content_text_view but after changing it the tests were passed but the UI had changed and the content_item was shrunk. Could you please guide on me how to solve this

  1. I think we dont' need to worry about the width. Just mentioned minWidth=48dp should be fine.
  2. Also for height also use minWidth instead of layout_height.
  3. If there is any extra margin around the TextView you can shift that to padding of TextView and that way the height should automatically get fixed (upto certain extent and not fully).
  4. As the changes in this PR deal with UI change please create before/after screenshot in mobile-portrait, landscape and tablet-portrait, landscape for Question Player as well as Exploration Player. Also as we are dealing with min-height based solution suggest picking an example for screenshot where the feedback item contains only 1 line of text so that we can correctly see the changes in before/after screenshots

Got it, Thanks!!

@Rohit1173
Copy link
Contributor Author

@rt4914 PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned Rohit1173 Feb 15, 2022
@oppiabot
Copy link

oppiabot bot commented Feb 15, 2022

Unassigning @Rohit1173 since a re-review was requested. @Rohit1173, please make sure you have addressed all review comments. Thanks!

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@Rohit1173 Suggested changes:

  1. This PR is part of [A11y] Enable AccessibilityChecks #3245 and does solve the entire issue. So please rename this to "Fix part of " in title and description.
  2. This PR focuses on Exploration Player only. You should also check this for Question Player.
  3. Have a look at [A11Y] Feedback item should be atleast 48dp in size #3927 as you are actually solving that issue too (sort of duplicate). Make sure that QuestionPlayerTests also pass and also share the before/after screenshot of question player UI for this particular test in [A11Y] Feedback item should be atleast 48dp in size #3927 is focused on.

From code wise it looks correct but we should still check UI and question player.

@rt4914 rt4914 assigned Rohit1173 and unassigned rt4914 Feb 15, 2022
@Rohit1173 Rohit1173 changed the title Fix #3245 [A11y] Enabling AccessibilityChecks for QuestionPlayerActivityTest Fix Part of #3245 [A11y] Enabling AccessibilityChecks for QuestionPlayerActivityTest Feb 17, 2022
@Rohit1173 Rohit1173 changed the title Fix Part of #3245 [A11y] Enabling AccessibilityChecks for QuestionPlayerActivityTest Fix part of #3245 [A11y] Enabling AccessibilityChecks for QuestionPlayerActivityTest Feb 17, 2022
@Rohit1173
Copy link
Contributor Author

@rt4914 updated the screenshots, PTAL, Thanks!

@oppiabot oppiabot bot assigned rt4914 and unassigned Rohit1173 Feb 17, 2022
@oppiabot
Copy link

oppiabot bot commented Feb 17, 2022

Unassigning @Rohit1173 since a re-review was requested. @Rohit1173, please make sure you have addressed all review comments. Thanks!

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 merged commit b091bd0 into oppia:develop Feb 22, 2022
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.

[A11Y] Feedback item should be atleast 48dp in size
2 participants