-
Notifications
You must be signed in to change notification settings - Fork 499
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 #4017: Radio Button Implementation #4225
Conversation
Once all tests in action pass, I will add the UI-related stuff in the PR description. |
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 @anandwana001! I think this looks really good. Just had a few comments--PTAL.
.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/player/state/StateFragmentTest.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.
LGTM, Thanks @anandwana001
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 @anandwana001. Just had two nits, otherwise the PR LGTM.
.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.kt
Show resolved
Hide resolved
.../main/java/org/oppia/android/app/player/state/itemviewmodel/SelectionInteractionViewModel.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 @anandwana001. LGTM!
Merging since it seems all approvals have been met, and no remaining conversations require resolution. |
Explanation
Fix #4017
Earlier, the MCQ questions which can accept only 1 answer do not contain the submit button and worked on the concept of direct selection to check if the answer is correct or not. (Selection is the submission)
But, for the questions which can accept more than 1 answer do have a submit answer button.
This PR makes the submit button available for former questions also which can accept only 1 answer.
Testing
I had added a few tests which test the single selection (Radio buttons) in multiple conditions.
I had tested espresso tests, for robolectric, I am relying on GitHub actions.
UI Related Items
Before
After
All the newly added tests passing on espresso.
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: