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 #3122, #4600: Dark mode implementation - Home Screen, Recently Played Screen, Promotional Cards, Profile Progress, Ongoing Topic List, Completed Story List #4786

Merged
merged 53 commits into from
Dec 18, 2022

Conversation

MohitGupta121
Copy link
Member

@MohitGupta121 MohitGupta121 commented Dec 12, 2022

Explanation

Dark mode implementation - Home Screen, Recently Played Screen, Promotional Cards, Profile Progress, Ongoing Topic List, Completed Story List

Fixes #3122 : Dark theme make colors of the images distorted
Fixes #4600 : Doesn't have dark mode mock for promoted story #cards

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

Home Screen

Recently Played Screen

Promotional Screen

Profile Progress

Ongoing Topic List

Completed Story List

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

@MohitGupta121
Copy link
Member Author

@rt4914 thanks for this review, I think we have to solve this issue in this PR it self as we have to complete the dark mode project.
But what you thinks? We file new issue or in same PR?

@rt4914
Copy link
Contributor

rt4914 commented Dec 15, 2022

@rt4914 thanks for this review, I think we have to solve this issue in this PR it self as we have to complete the dark mode project. But what you thinks? We file new issue or in same PR?

@MohitGupta121 In that case, let me review this PR first, apply those changes first and in last commit make these corner-issue 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.

@MohitGupta121 I have suggested all changes. PTAL at those changes (don't regenerate screenshots right now we will do that after the last commit).

app/src/main/res/layout/completed_story_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/ongoing_topic_item.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_summary_view.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/promoted_story_card.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/promoted_story_card.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/promoted_story_list.xml Outdated Show resolved Hide resolved
@MohitGupta121
Copy link
Member Author

MohitGupta121 commented Dec 15, 2022

@rt4914 I commit the requested changes and leave comments on your review can you PTAL on that comments.

@rt4914
Copy link
Contributor

rt4914 commented Dec 16, 2022

@MohitGupta121 I have replied to all the comments.

@MohitGupta121 Also, you had not replied to all the comments earlier. For now I have replied as "Done" and resolved them but please make sure that you reply to all comments otherwise your PR will get blocked on "Unresolved conversations".

@rt4914 rt4914 assigned MohitGupta121 and unassigned rt4914 Dec 16, 2022
@MohitGupta121
Copy link
Member Author

@MohitGupta121 I have replied to all the comments.

@MohitGupta121 Also, you had not replied to all the comments earlier. For now I have replied as "Done" and resolved them but please make sure that you reply to all comments otherwise your PR will get blocked on "Unresolved conversations".

Okay sure, I take care of this next time.
But I not have to make it as resolved?

@MohitGupta121
Copy link
Member Author

MohitGupta121 commented Dec 16, 2022

@rt4914 I done with the lastest requested changes. Can you PTAL.

@rt4914 In this comment I taking about the stroke image is set from the logical files. So my question is we have to set dark mode colors in Test file ImageViewBindingAdaptersTest.kt also?
And the stroke of the profile image set from the circular_stroke_2dp_grey_32dp.xml which is used in ImageViewBindingAdapters.java in line 100 and 103.

@rt4914
Copy link
Contributor

rt4914 commented Dec 18, 2022

circular_stroke_2dp_grey_32dp

  1. We don't need to apply dark-mode changes to Test files. Its completely optional.
  2. In circular_stroke_2dp_grey_32dp there multiple colors which are not supporting dark-mode and also are not from component_colors.xml files so this needs to be fixed.
  3. We should always updated update adapter files like ImageViewBindingAdapter to support dark mode and correct color variables.
  4. Most importantly the line (100-103) and file that you are mentioning, ie.e circular_stroke_2dp_grey_32dp it getting used in StoryActivity so it should not be part of this PR.

So I am just confused why are you asking question related to StoryActivity in this PR which has nothing to do with StoryActivity.

Am I missing something else here?

@rt4914 rt4914 assigned MohitGupta121 and unassigned rt4914 Dec 18, 2022
@MohitGupta121
Copy link
Member Author

MohitGupta121 commented Dec 18, 2022

Am I missing something else here?

Okay I just see that file that's why I have confused is this not a part of this PR.
Now it's clear to me, I have to done this fix circular_stroke_2dp_grey_32dp in the part of StoryActivity?

@rt4914
Copy link
Contributor

rt4914 commented Dec 18, 2022

Am I missing something else here?

Okay I just see that file that's why I have confused is this not a part of this PR. Now it's clear to me, I have to done this fix circular_stroke_2dp_grey_32dp in the part of StoryActivity?

Yes that's correct.

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, Thanks.

@rt4914 rt4914 merged commit 0bf4736 into oppia:develop Dec 18, 2022
@MohitGupta121
Copy link
Member Author

MohitGupta121 commented Dec 18, 2022

In dark mode if you notice closely at the edges, you will see that there are white corners which are visible. They should not be visible.

@rt4914 So now I start the corners fixes? As if everything is fine in this PR. And if we are in the last stage of this PR?

@MohitGupta121 MohitGupta121 deleted the dark-mode-implementation-4 branch December 18, 2022 08:11
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.

Doesn't have dark mode mock for promoted story cards Dark theme make colors of the images distorted
2 participants