Fix #161: Exploration player contentcard supports rich-text part -2 #229
Conversation
veena14cs
assigned rt4914 and nikitamarysolomanpvt and unassigned rt4914 and nikitamarysolomanpvt
app/src/main/java/org/oppia/app/player/content/ContentViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentListFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks @veena14cs! Overall structure looks good, but I have a view concerns about tests and how we're integrating the content list fragment. Most of the comments are various style nits.
app/src/main/java/org/oppia/app/player/content/ContentCardAdapter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentCardAdapter.kt
Outdated
Show resolved
Hide resolved
| ) | ||
| InteractionFeedbackViewHolder(binding) | ||
| } | ||
| else -> throw IllegalArgumentException("Invalid view type") |
There was a problem hiding this comment.
I suggest including the viewType parameter in this exception cause since it'll help provide context if the exception is ever thrown.
There was a problem hiding this comment.
I didn't understand what viewType parameter you mean. Can you please elaborate on this.
We have only two viewTypes over here one is content and other is learner's interaction.
app/src/main/java/org/oppia/app/player/content/ContentCardAdapter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentViewModel.kt
Outdated
Show resolved
Hide resolved
| if (getContentListFragment() == null) { | ||
| val contentListFragment = ContentListFragment() | ||
| val args = Bundle() | ||
| args.putString("exploration_id", dummyExploration_id) |
There was a problem hiding this comment.
Shouldn't we be using the same exploration ID that's being passed into StateFragmentPresenter?
If the explorations that are being included in the APK itself are not satisfactory for testing the state fragment, feel free to modify them to add additional rich text contents as needed.
There was a problem hiding this comment.
Yes
Shouldn't we be using the same exploration ID that's being passed into StateFragmentPresenter?
Yes we could use. I added dummy exploration id for testing image and modified json.
| ) | ||
|
|
||
| @Test | ||
| fun testContentListFragment_loadHtmlContent_isDisplayed() { |
There was a problem hiding this comment.
This is only verifying that the recycler view is visible, but not that any specifically correct HTML is also displayed. Please add tests to verify that various content card cases work correctly by verifying that the correct HTML is displayed:
- One content card
- One content card with interaction feedback
- One content card, one interaction feedback, a second content card
- Two pairs of content cards and interaction feedbacks
Also, I suggest verifying that a configuration change properly keeps the recycler view states across rotations with a fifth test.
There was a problem hiding this comment.
@BenHenning Actually adding interaction feedback depends on Continue/End Exploration ( #222 ),
so I think maybe you can finish reviewing this #222 and then she can write test-cases for this, or else if you have something else in mind, can you explain the alternate strategy.
|
@veena14cs Are you still working on this PR ? |
No, I am not. |
In such cases make sure you unassign reviewers because otherwise they will review this PR without having any knowledge of not reviewing this PR. Also, if possible, close this PR. |
|
This approach is changed. Fix is in the PR #245. |
Explanation
This PR is replica of #189 . It includes rich-text component that parses HTML content. This PR also extracts image from the HTML content.