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 #4710 Resume Lesson Fragment Button Responsiveness, Icon Layout, and Dark Mode Color #5116

Merged
merged 18 commits into from
Aug 11, 2023

Conversation

XichengSpencer
Copy link
Collaborator

@XichengSpencer XichengSpencer commented Aug 2, 2023

Fix #4710 the button scaling problem in largest text setting in the resume fragment, and fix the color change in dark mode.

The use of material button has better control for the icon inside the button so that it can group icons and text together as responsive design guidelines suggested. Particularly the icon gravity part that attached the icon to the text. Also, add flex layout so that button will wrap to two lines when the screen is too small.

Set app:backgroundTint = "@null" fixes the auto color change made by the app in the dark mode.

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:

Here are the testing on my own cell phone, even the second smallest text is too large for the screen so the buttons are wrapped in two rows. Only the smallest text can fit.

Screenshot 1 Screenshot 2 Screeshot 3
Weixin Image_20230606114148 Weixin Image_20230606114203 Weixin Image_20230606114209
  • Add a screenshot demonstrating UI of tablet
    |Screenshot 1| Screenshot 2|
    |--|--|
    |Screenshot 2023-06-05 120237|Screenshot 2023-06-05 120223|

LTR

Screenshot 1 Screenshot 2 Screeshot 3
Image_20230606114738 Image_20230606114706 Image_20230606114719
  • Dark Mode Fix
    | Before | After |
    |:------:|:-----:|
    | image | image |

|image|image|

Copy link
Member

@MohitGupta121 MohitGupta121 left a comment

Choose a reason for hiding this comment

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

@XichengSpencer LGTM, Thanks!

@MohitGupta121 MohitGupta121 removed their assignment Aug 2, 2023
Copy link
Collaborator

@kkmurerwa kkmurerwa left a comment

Choose a reason for hiding this comment

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

Hi @XichengSpencer. The PR looks good to me. I have a few issues with the PR title and description. The title could use some more explanation. Something like Fix #4710: Button Scaling and Dark Mode issues on Resume Lesson Fragment. The description should give a general explanation of the changes the fix makes. See PR #5098, #5115, and #5111 for reference.

@XichengSpencer XichengSpencer changed the title Fix #4710 Fix #4710 Resume Lesson fragment button responsiveness, icon layout, and dark mode color correction Aug 7, 2023
@XichengSpencer XichengSpencer changed the title Fix #4710 Resume Lesson fragment button responsiveness, icon layout, and dark mode color correction Fix #4710 Resume Lesson Fragment Button Responsiveness, Icon Layout, and Dark Mode Color Aug 7, 2023
@adhiamboperes
Copy link
Collaborator

Hi @XichengSpencer, is this PR ready for my review?

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.

Thanks @XichengSpencer!

This LGTM.

@adhiamboperes adhiamboperes enabled auto-merge (squash) August 11, 2023 11:04
@oppiabot oppiabot bot added the PR: LGTM label Aug 11, 2023
@oppiabot
Copy link

oppiabot bot commented Aug 11, 2023

Hi @XichengSpencer, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks!

@adhiamboperes adhiamboperes merged commit bb8f34e into oppia:develop Aug 11, 2023
36 checks passed
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 support for scaling in resume lesson screen
4 participants