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 #4586: Improve text scaling according to accessibility scanner #4587

Merged
merged 8 commits into from
Sep 20, 2022

Conversation

vrajdesai78
Copy link
Contributor

@vrajdesai78 vrajdesai78 commented Sep 11, 2022

Explanation

Fixes #4586: Improved text scaling by setting layout_height and layout_width to wrap_content and setting min_height = 48dp and min_width = 48dp (may vary in some cases).

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).

A11YS result before changes

image
image
image

A11Ys result after changes

image

image

@vrajdesai78
Copy link
Contributor Author

vrajdesai78 commented Sep 12, 2022

@rt4914 Test case is failing because back button and navigation drawer icon is not displayed completely. With the current approach, I am setting minHeight=48dp and layout_height=wrap_content. The problem with this approach is that back button and navigation drawer icon is not set vertically. To set it vertically we need to remove minHeight attribute or need to set minHeight and layout_height same.

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.

@vrajdesai78 A lot of changes in this PR seems non-required.

For example:

If in current develop code we have attributes

android:layout_height="?attr/actionBarSize"
android:minHeight="?attr/actionBarSize"

than the simplest and most correct solution for that is

android:layout_height="wrap_content"
android:minHeight="?attr/actionBarSize"

Also, I am not getting any error for navigation drawer except for the overall width of the entire navigation drawer, nothing related to icons in toolbar/actionbar.

Please do check app and your changes once again.

Also,

  • if anywhere scanner tests are failing, attach screenshot with correct committed code.
  • if anywhere UI is not looking good as you mentioned, please share screenshot of that too.

@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Sep 12, 2022
@vrajdesai78 vrajdesai78 assigned rt4914 and unassigned vrajdesai78 Sep 12, 2022
@rt4914
Copy link
Contributor

rt4914 commented Sep 14, 2022

@vrajdesai78
I haven't tried these solutions but I think this should work

Solution 1

using minWidth="<whatever_the_current_fixed_width_is>"
+
layout_width="wrap_content"
This should mostly solve this error.

Solution 2

Reference

Just a sample code, not perfect, check mocks for perfect code. This sample code is for button.

paddingTop=8dp
paddingBottom=8dp
paddingStart=8dp
paddingEnd=8dp
layout_width="wrap_content"
layout_height="wrap_content"
minHeight=48dp
minWidth=48dp

Solution 3

In this also the layout_height is fixed, simply change that to wrap_content

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.

@vrajdesai78 Please mention what issues you are still facing in this PR.

Earlier I had a look at PR description from which I understood that you are still stuck on errors but from code it looks like you are not.

So if you are stuck on something explicitly mention that.

Otherwise I have mentioned the approach for buttons, please follow that for better results.

app/src/main/res/layout-land/resume_lesson_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-land/resume_lesson_fragment.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Sep 14, 2022
@vrajdesai78 vrajdesai78 assigned rt4914 and unassigned vrajdesai78 Sep 15, 2022
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.

@vrajdesai78 Requested some explanations. PTAL

app/src/main/res/layout/app_language_activity.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/promoted_story_card.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/promoted_story_card.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Sep 16, 2022
@vrajdesai78 vrajdesai78 assigned rt4914 and unassigned vrajdesai78 Sep 17, 2022
@rt4914 rt4914 assigned vrajdesai78 and unassigned rt4914 Sep 19, 2022
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 enabled auto-merge (squash) September 20, 2022 16:08
@oppiabot oppiabot bot unassigned rt4914 Sep 20, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 20, 2022

Unassigning @rt4914 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Sep 20, 2022
@rt4914 rt4914 merged commit 91014af into oppia:develop Sep 20, 2022
BenHenning added a commit that referenced this pull request Oct 31, 2022
BenHenning added a commit that referenced this pull request Nov 1, 2022
## Explanation
Fixes #4684

This PR reverts #4587. I think the change of match_parent to
wraps_content caused the promoted story container to grow for larger
images, and this wasn't caught until we tried the changes with
production assets. I noticed other places in the PR that might have the
same issue, so rather than trying to make sense of everything in order
to properly fix it & add tests, I opted to revert the change since the
regression is beta-blocking (even though it wasn't a clean reversion and
required manual changes).

Note that the reversion includes keeping the changes to
recently_played_fragment since otherwise changes in
RecentlyPlayedFragmentTest fail, and the change seems unlikely to cause
any problems with respect to potential thumbnail sizing issues.

## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
There's not much to show here--see the issue for context on the problem.
The post-reversion just reverts things back to their previous state
(i.e. no regression).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve text scaling according to accessibility scanner
2 participants