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 #4950 : Audio Language Dialog Items and Hints & Solution link text Dark Mode #4946

Merged
merged 6 commits into from
Apr 21, 2023
Merged

Fix #4950 : Audio Language Dialog Items and Hints & Solution link text Dark Mode #4946

merged 6 commits into from
Apr 21, 2023

Conversation

MohitGupta121
Copy link
Member

@MohitGupta121 MohitGupta121 commented Apr 14, 2023

Explanation

Fix #4950 : Audio Language Dialog Items and Hints & Solution link text 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

Audio Language Dialog

Before - Light Mode Before - Dark Mode
After - Light Mode After - Dark Mode

Currently Offline Dialog

Before - Light Mode Before - Dark Mode
After - Light Mode After - Dark Mode

Currently on Cellular Data Dialog

Light Mode Dark Mode
Light Mode Dark Mode

Profile Picture Dialog

Light Mode Dark Mode

Leave to Topic Page Dialog

Light Mode Dark Mode

Show Solution Dialog

Light Mode Dark Mode

Logout Alert Dialog

Light Mode Dark Mode

Mark Chapter Complete Dialog

Light Mode Dark Mode

Exit Profile Dialog

Light Mode Dark Mode

Pin Detail Dialog

Light Mode Dark Mode

Forgot Pin Dialog

Light Mode Dark Mode

Confirm Oppia Data Reset Dialog

Light Mode Dark Mode

Access to Administrator Settings Dialog

Light Mode Dark Mode

Maximum storage capacity reached Dialog

Light Mode Dark Mode

Hints & Solution

Before - Light Mode Before - Dark Mode
After - Light Mode After - Dark Mode

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

Copy link
Member Author

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

@seanlip @BenHenning @rt4914 PTAL, fix the issue that Sean mail today.
Thanks

@seanlip seanlip removed their assignment Apr 14, 2023
@MohitGupta121 MohitGupta121 changed the title Fixed : Audio Language Dialog Items and Hints & Solution link text Dark Mode Fix Audio Language Dialog Items and Hints & Solution link text Dark Mode Apr 14, 2023
@MohitGupta121 MohitGupta121 changed the title Fix Audio Language Dialog Items and Hints & Solution link text Dark Mode Fix : Audio Language Dialog Items and Hints & Solution link text Dark Mode Apr 14, 2023
Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @MohitGupta121 for the fast turnaround here! Is there an issue tracking this? If not, it'd be good to have one to contextualize the problem itself.

Otherwise, had a few comments--PTAL.

app/src/main/res/values/themes.xml Show resolved Hide resolved
app/src/main/res/layout/hint_summary.xml Show resolved Hide resolved
@MohitGupta121
Copy link
Member Author

@BenHenning PTAL on review comments, Thanks,
I update the PR description added Before and After, adding all dialogs screenshot by today.
Thanks.

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Apr 17, 2023

Hey @MohitGupta121 Wondering if we might also want to make the unselected radio buttons white in dark mode?

Audio Language Dialog

After - Light Mode After - Dark Mode

@MohitGupta121
Copy link
Member Author

@adhiamboperes yes good suggestion, thanks.
I will do it.

@MohitGupta121 MohitGupta121 self-assigned this Apr 17, 2023
@MohitGupta121 MohitGupta121 removed their assignment Apr 17, 2023
@MohitGupta121
Copy link
Member Author

@adhiamboperes Done, PTAL on PR description. I updated the screenshots and code.
Thanks for this suggestion.

@seanlip seanlip assigned BenHenning and unassigned rt4914 and seanlip Apr 18, 2023
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.

Thanks @MohitGupta121, no concerns from my end.

@BenHenning are you OK approving this?

@seanlip
Copy link
Member

seanlip commented Apr 18, 2023

@MohitGupta121 I do have one (optional) suggestion. Notice how the hint containers have separate colours for the title and body of each, in light mode. This is absent in your dark mode mocks. Is there an existing pattern for this in dark mode that you can follow here?

If not, no worries, I don't think it's too serious and we can look at it again later if it causes problems.

@MohitGupta121
Copy link
Member Author

This generally LGTM, but is there a tracking issue for this? Also, I'm wondering if the PR description & title should be updated to make it more clear that this affects all alert dialogs.

@BenHenning at present there is no such issue for this PR, we need issue for these? If you suggest I open one issue for these changes.

Regarding all the Alerts dialogs screenshot, today by EOD I will update the PR description with all screenshots of Dialogs. (As today my exams totally finished)

@MohitGupta121
Copy link
Member Author

@seanlip Actually, we follows the mock of hint and solution that's why according to mock there is no such container.
But if you suggest we can go ahead with the container by getting reference from other mocks and from suggestions of @rt4914 and Design Team.

@seanlip
Copy link
Member

seanlip commented Apr 18, 2023

@MohitGupta121 I think that mock is an old version which didn't have a divider between the title and body sections.

But, in any case, I would suggest leaving it for now if there's no obvious colour to use -- we can tackle it later!

@MohitGupta121 MohitGupta121 removed their assignment Apr 18, 2023
Copy link
Member Author

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

@BenHenning PTAL, Thanks.
I added all alert dialogs screenshots in the PR description.

Also fixed one Offline-Dialog in dark-mode (convert to androidx) as mentioned in Before screenshot what the exact issue.

@adhiamboperes
Copy link
Collaborator

What does the edittext look like when in focus? And is the text legible with this background color?

@MohitGupta121 MohitGupta121 changed the title Fix : Audio Language Dialog Items and Hints & Solution link text Dark Mode Fix #4950 : Audio Language Dialog Items and Hints & Solution link text Dark Mode Apr 18, 2023
@MohitGupta121
Copy link
Member Author

MohitGupta121 commented Apr 18, 2023

@adhiamboperes This is when the EditText is not in focus, actually the text is the hint text that itself on the text field.

This one when in focus:

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Apr 18, 2023

@MohitGupta121, looks okay to me.

Copy link
Sponsor Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Thanks @MohitGupta121! This looks quite good--glad that we caught this before advertising the launch of dark mode. :)

@BenHenning
Copy link
Sponsor Member

Updating to the latest develop & enabling auto-merge.

@BenHenning BenHenning enabled auto-merge (squash) April 21, 2023 02:42
@oppiabot
Copy link

oppiabot bot commented Apr 21, 2023

Unassigning @BenHenning since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Apr 21, 2023
@oppiabot
Copy link

oppiabot bot commented Apr 21, 2023

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

@BenHenning BenHenning merged commit 3b7e1a4 into oppia:develop Apr 21, 2023
36 checks passed
@MohitGupta121 MohitGupta121 deleted the fixed_mail_issues_by_sean branch April 21, 2023 03:37
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.

Dark mode bugs in Alert Dailogs
5 participants