-
Notifications
You must be signed in to change notification settings - Fork 528
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
Fix part of #632: Refactor topic list adapter to use bindable adapter #2246
Fix part of #632: Refactor topic list adapter to use bindable adapter #2246
Conversation
…motedStoryList layout
app/src/main/java/org/oppia/android/app/home/topiclist/TopicSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/topiclist/TopicSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/topiclist/TopicSummaryViewModel.kt
Outdated
Show resolved
Hide resolved
…ist-adapter-to-use-bindable-adapter
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 @jcqli. I'm confused by some of the most recent changes & left comments accordingly.
app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/home/promotedlist/PromotedStoryListView.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/topic/StoryProgressTestHelper.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/promotedlist/PromotedStoryListView.kt
Outdated
Show resolved
Hide resolved
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 @jcqli! LGTM!
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.
LGTM, thanks.
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.
nit changes. @jcqli you can resolve my comments once done.
LGTM
...rc/sharedTest/java/org/oppia/android/app/home/promotedlist/PromotedStoryListViewModelTest.kt
Show resolved
Hide resolved
@veena14cs Those this is not merged yet as there are 2 nit changes pending. But now you can update your PR fully and it won't create any major conflicts after the nits are solved. Or you can wait for 1 more day and get this code from develop. |
@rt4914 How about I will shift this nit changes in veena's pr, this way we can merge this PR now? |
In @veena14cs 's PR we won't be able to see these changes as they are not done by her. Instead we can file and issue and merge this. I am doing it. Thanks. |
#2503 |
Explanation
Fix part of #632 by replacing TopicListAdapter with a BindableAdapter implementation. Adds a HomeViewModel to perform data processing of WelcomeViewModel, PromotedStory view models, and TopicSummary view models all at the same time so child layouts appear with home fragment. Also fixes a data binding for topic_lessons_story_summary that was causing app to crash on tablet and landscape phone orientation.
Checklist