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.

Sign up for GitHub

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

Jump to bottom

Fixes#4184: Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity #4289

Merged
merged 19 commits into from
May 27, 2022
Merged

Fixes#4184: Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity #4289

merged 19 commits into from
May 27, 2022

Conversation

aayushimathur6
Copy link
Collaborator

@aayushimathur6 aayushimathur6 commented Apr 9, 2022 •
edited

Explanation

Fixes #4184

Mocks Link:

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
First Header Second Header
Screenshot_20220514_185423 Screenshot_20220514_185517
Screenshot_20220514_185920 Screenshot_20220514_185834
Screenshot_20220515_201854 Screenshot_20220515_201650
Screenshot_20220416_191537 Screenshot_20220416_191251
Screenshot_20220515_201955 Screenshot_20220515_201745
Screenshot_20220423_184652 Screenshot_20220423_184412
Screenshot_20220424_140509 Screenshot_20220423_185116

@aayushimathur6
Copy link
Collaborator Author

@ayush0402 I have added the dark mode in third party dependency list activity but for license activity, I was unable to do so as I was a bit confused about where to implement it and what colors & also dark mode mocks in wiki don't have images regarding license activity.

@aayushimathur6 aayushimathur6 changed the title add dark mode to third party dependency list activity Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity #4184 Apr 9, 2022
@aayushimathur6 aayushimathur6 changed the title Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity #4184 Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity Apr 9, 2022
@aayushimathur6 aayushimathur6 changed the title Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity Fixes#4184: Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity Apr 9, 2022
@BenHenning
Copy link
Sponsor Member

@rt4914 for this PR I think Gradle just sort of awkwardly died in shard 2 (there were no failure logs uploaded, either). I'm just restarting it, though it seems unrelated to the failure in #4268.

@aayushimathur6
Copy link
Collaborator Author

@rt4914 PTAL Thanks!

@BenHenning BenHenning self-assigned this Apr 19, 2022
@BenHenning
Copy link
Sponsor Member

@aayushimathur6 can you please also add screenshots for the license text view activity? You have some for the list views, but not the one that shows actual license text.

@ayush0402 could you take a first pass review on this?

@aayushimathur6
Copy link
Collaborator Author

@BenHenning added the asked PTAL thanks!

@aayushimathur6
Merge branch 'develop' of https://github.com/oppia/oppia-android into…
3fdecb0 
… AdddarkmodesupporttoThirdPartyDependencyListActivity,LicenseListActivityandLicenseTextViewerActivity
@aayushimathur6
updated the error in license text viewer activity
03863f5
@oppiabot
Copy link

oppiabot bot commented May 18, 2022

Unassigning @Akshatkamboj14 since they have already approved the PR.

@oppiabot
Copy link

oppiabot bot commented May 18, 2022

Assigning @BenHenning for code owner reviews. Thanks!

@aayushimathur6
Copy link
Collaborator Author

@aayushimathur6 When I am checking the UI in current app on develop, there are divider lines in between each item. Please have a look and fix them

done

bhaktideshmukh
bhaktideshmukh approved these changes May 18, 2022
View reviewed changes
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, Thanks.

@bhaktideshmukh bhaktideshmukh removed their assignment May 18, 2022
rt4914
rt4914 suggested changes May 24, 2022
View reviewed changes
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.

@aayushimathur6 PTAL Thanks.

app/src/main/res/layout/license_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/license_text_viewer_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/license_text_viewer_fragment.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/third_party_dependency_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/third_party_dependency_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/third_party_dependency_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/third_party_dependency_item.xml Outdated Show resolved Hide resolved
app/src/main/res/values/component_colors.xml Outdated Show resolved Hide resolved
@aayushimathur6
Copy link
Collaborator Author

@rt4914 updated all the asked changes, PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned aayushimathur6 May 25, 2022
@oppiabot
Copy link

oppiabot bot commented May 25, 2022

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

rt4914
rt4914 approved these changes May 27, 2022
View reviewed changes
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.

@aayushimathur6 One nit change, otherwise LGTM

app/src/main/res/layout/third_party_dependency_item.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned aayushimathur6 and unassigned rt4914 May 27, 2022
@oppiabot
Copy link

oppiabot bot commented May 27, 2022

Assigning @BenHenning for code owner reviews. Thanks!

@aayushimathur6
Copy link
Collaborator Author

@rt4914 done with last change

@rt4914 rt4914 enabled auto-merge (squash) May 27, 2022 19:55
@rt4914 rt4914 assigned rt4914 and unassigned aayushimathur6 May 27, 2022
@rt4914 rt4914 merged commit 7d31a49 into oppia:develop May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@bhaktideshmukh bhaktideshmukh bhaktideshmukh approved these changes

@Akshatkamboj14 Akshatkamboj14 Akshatkamboj14 approved these changes

@BenHenning BenHenning Awaiting requested review from BenHenning

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add dark mode support to ThirdPartyDependencyListActivity, LicenseListActivity and LicenseTextViewerActivity
6 participants
@aayushimathur6 @BenHenning @bhaktideshmukh @rt4914 @Akshatkamboj14 @ayush0402