-
Notifications
You must be signed in to change notification settings - Fork 527
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
Oppia Android Promoted Stories Domain Layer #2272
Conversation
… github.com:jcqli/oppia-android into refactor-topic-list-adapter-to-use-bindable-adapter
This reverts commit f090330.
@veena14cs Most of implementation is changing in the below-mentioned PR for Home RecyclerView, as shifting from Adapter to Bindable Adapter. |
…ontroller.kt Co-authored-by: Ben Henning <[email protected]>
…ndroid into promoted-story
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Outdated
Show resolved
Hide resolved
…ontroller.kt Co-authored-by: Ben Henning <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs! Only nits left. I verified all the tests--they look correct to me now. Glad to see the tests passing as well. I think once the leftover comments are addressed this can be merged.
domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/topic/TopicListControllerTest.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt
Outdated
Show resolved
Hide resolved
…ontrollerTest.kt Co-authored-by: Ben Henning <[email protected]>
…ontrollerTest.kt Co-authored-by: Ben Henning <[email protected]>
…ontroller.kt Co-authored-by: Ben Henning <[email protected]>
Thanks @veena14cs. As mentioned in chat, please never force push as it can ruin the PR review. In this case, I checked the forced push commits & the new commits--it seems like the current history is fine. In the future, you can avoid force pushing by performing similar operations that add onto the branch history rather than change it (e.g. cherry-pick, or manually Regarding the PR, it generally LGTM at this state. I want to make sure the CI checks pass before approving, though, since it seems like there was 1 test failure in the latest commit. |
Ah nevermind, it looks like my view of GitHub was just out of date. Everything seems passing now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @veena14cs! LGTM.
Note that I'm specifically giving an exception to auto-closing this PR per the force-push due to it not actually affecting the reviewer functionality in this case--the final deltas still look good to me.
Merging this. |
Thanks Ben! there was no any major change. Will make sure to not to force push anytime. |
Explanation
This PR shows a list of promoted stories on the home screen. This implemenation is Domain layer part and App layer implementation is in PR #2320
The list(referred to as promoted stories list in code) can show one of the following:
The number of cases for Stories for you
The number of cases for Recommended stories
The number of cases for Recently-played/Last-played stories
Coming Soon topics
Design Document
https://docs.google.com/document/d/1o7hOz1rP-hJaKn1f5x6LV6ms0bzhyCpsTifJQY5crCc/edit?ts=5f9fc81b#heading=h.ovlu0iazlug6
Note:
This PR is only focusing on domain layer. As there are high-end changes and app layer CI check are failing. This errors are fixed in PR #2320 .
Checklist