-
Notifications
You must be signed in to change notification settings - Fork 521
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 #4735 Text unit is set in dp instead of sp #5134
Conversation
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 that this is definitely a good approach.
The screenshots that you have shared don't seem to be from the accessibility scanner. Please refer to our wiki section on accessibility if you need help with that.
app/src/main/java/org/oppia/android/app/profile/AdminPinActivityPresenter.kt
Outdated
Show resolved
Hide resolved
Hi @Vishwajith-Shettigar, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
Hey please tell is this issue android 12 specific, I haven't tried accessability Scanner with 12 |
Basically;
|
Your changes have led to some failing tests, here is information on how to find what is broken, so you can update the code or tests as appropriate. |
After fix |
The screenshots look good. Thanks! |
Hi @Vishwajith-Shettigar, once you are done making changes, please do the following:
|
hey actually I'm not able to understand why my checks are failing, can you give some idea about this please, that would be very helpful. @BenHenning |
Unassigning @Vishwajith-Shettigar since a re-review was requested. @Vishwajith-Shettigar, please make sure you have addressed all review comments. Thanks! |
Can you guide me why some checks are failing please ? |
I have left one nit comment - and for complete approval please update your PR description to include the following:
|
I have made some changes PTAL, thank you. |
Thanks. Do you know why the lightmode screenshot of the "after" fix is showing new suggestions? |
no, I dont have any idea about that. |
Well, could you please share the suggestions? |
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 for 1 codeowner file
I checked this and the issue appears on the beta version as well. I don't think that should block this PR since the toolbar issue is fixed. @Vishwajith-Shettigar, do you mind creating a bug report for the contrast issue? @BenHenning, PTAL. |
@adhiamboperes Sure, thank you. |
Unassigning @Vishwajith-Shettigar since a re-review was requested. @Vishwajith-Shettigar, please make sure you have addressed all review comments. Thanks! |
Unassigning @adhiamboperes since they have already approved the PR. |
Merging this since a seperate issue #5142 has been created for the edit text contrasts. |
Explanation
Fix #4735 , Added toolbar in admin_pin_activity xml file inorder to control the title size using sp.
Essential Checklist
For UI-specific PRs only
Before fix
Admin pin activity
After fix
If your PR includes UI-related changes, then: