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 #4191: Added dark mode support to DeveloperOptionsActivity, ViewEventsLogsActivity and ForceNetworkTypeActivity #4308

Conversation

Akshatkamboj14
Copy link
Member

@Akshatkamboj14 Akshatkamboj14 commented Apr 16, 2022

Explanation

Fixes: #4191

Mocks-
View Events Logs Activity:- https://xd.adobe.com/view/c05e9343-60f6-4c11-84ac-c756b75b940f-950d/screen/f23e514d-6f03-412e-883d-09817a43ce1f/specs/

Developer Options Activity:- https://xd.adobe.com/view/c05e9343-60f6-4c11-84ac-c756b75b940f-950d/screen/1ade83cc-ed11-4861-bf2a-60089232fd44/specs/

Force Network Type Activity:- https://xd.adobe.com/view/c05e9343-60f6-4c11-84ac-c756b75b940f-950d/screen/79be23a6-c676-4266-9f6c-60ac8751c68a/specs/

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:

  • 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
Default Mode Dark Mode
image image
image image
image image
image image
image image

@Akshatkamboj14
Copy link
Member Author

@rt4914 @BenHenning @ayush0402 PTAL, Thanks!

Copy link
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 @Akshatkamboj14! I mainly had 1 comment in code, and one other question: shouldn't we also be updating the math expressions developer options view? It seems a bit odd to update everything except that, or is there a separate tracking issue for it?

Otherwise, PTAL.

@ayush0402 PTAL as well since this is a dark mode PR.

… Add-dark-mode-support-to-DevloperOptionsActivity-ViewEventsLogsActivity-and-ForceNetworkTypeActivity
@ayush0402
Copy link
Contributor

@BenHenning There is no tracking issue for math expressions developer options currently as this was added later.
@Akshatkamboj14 Do you think you can also include the layout under Math Expressions/Equations in this PR only?
Thanks.

@Akshatkamboj14
Copy link
Member Author

@ayush0402 actually I am not getting what Math Expressions/Equations mean in this issue.

@Akshatkamboj14
Copy link
Member Author

Hey, @BenHenning @ayush0402 can you tell me from where can I take reference to implement dark mode on
Math Expressions/Equations. I am not able to find the mocks regarding this.

@BenHenning
Copy link
Member

Sorry, I missed this earlier. @Akshatkamboj14 you can see the math & expressions screen by opening the developer options menu & scrolling to the bottom. In terms of mocks, you can follow the same styling as the other screens in developer options. Since these are developer-facing screens, it's less important to have perfect mocks. Feel free to ask if you're unsure about any particular parts of the screen though.

The reason I'm suggesting this is that I'm treating #4191 as introducing dark mode support for all of developer options--is that wrong @ayush0402?

… Add-dark-mode-support-to-DevloperOptionsActivity-ViewEventsLogsActivity-and-ForceNetworkTypeActivity
@Akshatkamboj14
Copy link
Member Author

hey, @rt4914 @ayush0402 @BenHenning I have committed the changes, PTAL.

@rt4914 rt4914 assigned Akshatkamboj14 and unassigned rt4914 May 17, 2022
Copy link
Contributor

@bhaktideshmukh bhaktideshmukh left a comment

Choose a reason for hiding this comment

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

LGTM

@bhaktideshmukh bhaktideshmukh removed their assignment May 18, 2022
@oppiabot
Copy link

oppiabot bot commented May 18, 2022

Assigning @BenHenning for code owner reviews. Thanks!

Copy link
Collaborator

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

@oppiabot
Copy link

oppiabot bot commented May 18, 2022

Unassigning @aayushimathur6 since they have already approved the PR.

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.

One very minor change. PTAL Thanks.

app/src/main/res/values/component_colors.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 and BenHenning May 24, 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

…ctivity-ViewEventsLogsActivity-and-ForceNetworkTypeActivity
…ctivity-ViewEventsLogsActivity-and-ForceNetworkTypeActivity
@BenHenning BenHenning dismissed their stale review June 1, 2022 23:26

Codeowners no longer needed.

@BenHenning
Copy link
Member

Apologies for the super long delay here. Since my original requests have been addressed, my codeowners is no longer needed, all open threads seem resolved, and all other reviewers seem happy with the PR, I'm going ahead and merging this. Thanks @Akshatkamboj14!

@BenHenning BenHenning merged commit 517964c into oppia:develop Jun 1, 2022
@Akshatkamboj14 Akshatkamboj14 self-assigned this Jun 19, 2022
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.

Add dark mode support to DevloperOptionsActivity, ViewEventsLogsActivity and ForceNetworkTypeActivity
6 participants