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 #4735 Text unit is set in dp instead of sp #5134

Merged
merged 12 commits into from
Sep 5, 2023

Conversation

Vishwajith-Shettigar
Copy link
Collaborator

@Vishwajith-Shettigar Vishwajith-Shettigar commented Aug 14, 2023

Explanation

Fix #4735 , Added toolbar in admin_pin_activity xml file inorder to control the title size using sp.

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

Before fix
Admin pin activity

Light mode
before light

After fix

Light mode Dark mode
afterlightmode afterdarkmode

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

@Vishwajith-Shettigar Vishwajith-Shettigar requested review from a team as code owners August 14, 2023 08:30
@Vishwajith-Shettigar Vishwajith-Shettigar changed the title Fix issue #4735 Text unit is set in dp instead of sp Fix #4735 Text unit is set in dp instead of sp Aug 14, 2023
Copy link
Collaborator

@adhiamboperes adhiamboperes left a 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.

@oppiabot
Copy link

oppiabot bot commented Aug 15, 2023

Hi @Vishwajith-Shettigar, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks!

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Aug 15, 2023

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.

Here is accessability scanner screenshot.

Android version 10
When I scanned, I have got no suggestions, please guide me if something is wrong here.
WhatsApp Image 2023-08-15 at 9 02 57 PM

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Aug 15, 2023

Hey please tell is this issue android 12 specific, I haven't tried accessability Scanner with 12
Edit: Attached 12 also

@Vishwajith-Shettigar
Copy link
Collaborator Author

Android 12
Accessability scanner screenshot

onemore

@adhiamboperes
Copy link
Collaborator

Here is accessability scanner screenshot.

Android version 10 When I scanned, I have got no suggestions, please guide me if something is wrong here.

Basically;

  1. You need to be able to verify that you can reproduce the issue. You can do this by switching to a branch like develop, deploying your app on that branch, then running the accessibility scanner for this screen. You should see the warnings same as in the issue description. (Save the screenshot, because you need to add it it to the description of this PR)
  2. Next, switch back to your branch(that contains your fix), and deploy the app, test it with the accessibility scanner. essentially, if your fix works, all the warnings should be gone(no suggestions). You will also add this screenshot to the PR description for before/after proof.

@adhiamboperes
Copy link
Collaborator

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.

@Vishwajith-Shettigar
Copy link
Collaborator Author

Here is accessability scanner screenshot.
Android version 10 When I scanned, I have got no suggestions, please guide me if something is wrong here.

Basically;

  1. You need to be able to verify that you can reproduce the issue. You can do this by switching to a branch like develop, deploying your app on that branch, then running the accessibility scanner for this screen. You should see the warnings same as in the issue description. (Save the screenshot, because you need to add it it to the description of this PR)
  2. Next, switch back to your branch(that contains your fix), and deploy the app, test it with the accessibility scanner. essentially, if your fix works, all the warnings should be gone(no suggestions). You will also add this screenshot to the PR description for before/after proof.

Here is before fix
befforetoolbar

After fix

onemore

@adhiamboperes
Copy link
Collaborator

The screenshots look good. Thanks!

@adhiamboperes
Copy link
Collaborator

Hi @Vishwajith-Shettigar, once you are done making changes, please do the following:

  • Go to the PR description and make sure you have done all the items on the Essential Checklist, and check them off
  • Assign the PR to Ben Henning by mentioning him with "@(Ben Henning), PTAL".

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Aug 19, 2023

Hi @Vishwajith-Shettigar, once you are done making changes, please do the following:

  • Go to the PR description and make sure you have done all the items on the Essential Checklist, and check them off
  • Assign the PR to Ben Henning by mentioning him with "@(Ben Henning), PTAL".

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

@oppiabot
Copy link

oppiabot bot commented Aug 19, 2023

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

@Vishwajith-Shettigar
Copy link
Collaborator Author

Can you guide me why some checks are failing please ?

@adhiamboperes
Copy link
Collaborator

I have left one nit comment - and for complete approval please update your PR description to include the following:

  • An explanation of the changes that you have made
  • Before(build using develop branch) and after screenshots(build app using your branch) of accessibility scanner showing that the warning is shown before your fix and gone after the fix. You can look at table formatting in markdown to organize your screenshots. Please see this for example.

@Vishwajith-Shettigar
Copy link
Collaborator Author

I have made some changes PTAL, thank you.

@adhiamboperes
Copy link
Collaborator

I have made some changes PTAL, thank you.

Thanks. Do you know why the lightmode screenshot of the "after" fix is showing new suggestions?

@Vishwajith-Shettigar
Copy link
Collaborator Author

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.

@adhiamboperes
Copy link
Collaborator

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?

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Sep 1, 2023

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?

seems like its about background and foreground contrast ratio.

sug1

sug2

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Sep 2, 2023

I switched to develop branch,now it shows same with light mode (api 31)
beforelightedittext

Copy link
Member

@seanlip seanlip left a 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

@adhiamboperes
Copy link
Collaborator

I switched to develop branch,now it shows same with light mode (api 31)

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.

@Vishwajith-Shettigar
Copy link
Collaborator Author

Vishwajith-Shettigar commented Sep 4, 2023

I switched to develop branch,now it shows same with light mode (api 31)

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.

@oppiabot
Copy link

oppiabot bot commented Sep 4, 2023

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

@oppiabot
Copy link

oppiabot bot commented Sep 5, 2023

Unassigning @adhiamboperes since they have already approved the PR.

@adhiamboperes
Copy link
Collaborator

Merging this since a seperate issue #5142 has been created for the edit text contrasts.

@adhiamboperes adhiamboperes enabled auto-merge (squash) September 5, 2023 20:15
@adhiamboperes adhiamboperes merged commit 7668bf0 into oppia:develop Sep 5, 2023
2 of 3 checks passed
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.

Text unit is set in dp instead of sp
5 participants