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

Fix #4448: Hide promoted story section for the first time user #4430

Merged
merged 22 commits into from
Aug 24, 2022
Merged

Fix #4448: Hide promoted story section for the first time user #4430

merged 22 commits into from
Aug 24, 2022

Conversation

JishnuGoyal
Copy link
Contributor

@JishnuGoyal JishnuGoyal commented Jul 12, 2022 •
edited

Explanation

This PR is a part of the GSoC project: Interactive Onboarding Flow which fixes #4448 by hiding the promoted stories section when the user logs in for the first time. A clear CTA is also added as the topics list header. Note that when the user enters the app for the second time (and every time after that), the promoted stories section is displayed as usual.

The hiding of the promoted stories section is achieved by utilizing the work in #4438 <--this PR where a login counter was introduced. Tests are modified to account for the hidden UI elements. New test cases are also added to back the functionality introduced in the PR.

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

Before:
image

After:
First login:
image

Second login:
image

@oppiabot
Copy link

oppiabot bot commented Jul 19, 2022

Hi @JishnuGoyal, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 19, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jul 23, 2022
@JishnuGoyal JishnuGoyal mentioned this pull request Jul 27, 2022
Closed
@oppiabot
Copy link

oppiabot bot commented Aug 3, 2022

Hi @JishnuGoyal, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 3, 2022
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 4, 2022
@JishnuGoyal JishnuGoyal changed the title hide promoted stories Fix #4448: Hide promoted story section for the first time user Aug 4, 2022
@JishnuGoyal JishnuGoyal marked this pull request as ready for review August 4, 2022 17:13
@JishnuGoyal
Copy link
Contributor Author

@BenHenning PTAL would be possible for you to take an initial pass at this? I'll be adding tests for the functionality in my next commit.

@JishnuGoyal
Copy link
Contributor Author

please ignore the first 4 commits as they were finally changed after merger with develop re: #4438

BenHenning
BenHenning requested changes Aug 5, 2022
View reviewed changes
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 @JishnuGoyal! Took a first pass--PTAL.

Also, please make sure to fill in the rest of the PR description (I think you're missing filling out the checklist, and the UI part of the PR description is missing).

app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/all_topics.xml Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Aug 5, 2022
@JishnuGoyal
Copy link
Contributor Author

@BenHenning PTAL

@JishnuGoyal
Copy link
Contributor Author

PTAL @rt4914

@oppiabot oppiabot bot assigned BenHenning and unassigned JishnuGoyal Aug 16, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 16, 2022

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

rt4914
rt4914 approved these changes Aug 18, 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.

LGTM for codeowners

@oppiabot oppiabot bot unassigned rt4914 Aug 18, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 18, 2022

Unassigning @rt4914 since they have already approved the PR.

BenHenning
BenHenning reviewed Aug 22, 2022
View reviewed changes
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 @JishnuGoyal! Just had one final comment--PTAL.

app/src/sharedTest/java/org/oppia/android/app/home/HomeActivityTest.kt Outdated Show resolved Hide resolved
@BenHenning BenHenning assigned JishnuGoyal and unassigned BenHenning Aug 22, 2022
@JishnuGoyal
Copy link
Contributor Author

Yes, this has been resolved and was added to a separate function. DataProviderTestMonitor was used instead of the legacy code, as discussed in the meeting.
PTAL @BenHenning

@oppiabot oppiabot bot assigned BenHenning and unassigned JishnuGoyal Aug 22, 2022
@oppiabot
Copy link

oppiabot bot commented Aug 22, 2022

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

BenHenning
BenHenning approved these changes Aug 24, 2022
View reviewed changes
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 @JishnuGoyal. This LGTM!

@BenHenning BenHenning merged commit 76f954d into oppia:develop Aug 24, 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

@BenHenning BenHenning BenHenning approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hide promoted story section for the first time user
3 participants
@JishnuGoyal @rt4914 @BenHenning