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 #156 & #157: Continue/End Exploration player buttons- low-fi #222

Closed
wants to merge 13 commits into from
Closed

Fix #156 & #157: Continue/End Exploration player buttons- low-fi #222

wants to merge 13 commits into from

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Oct 10, 2019

Explanation

This fixes #156 and #157 and introduces following buttons which will be used in state:

  1. Continue
  2. Return to Topic
  3. Learn Again
  4. Submit
  5. Previous Arrow Button
  6. Next Arrow Button

Reference Design Doc: https://docs.google.com/document/d/1iD4sY_hWmWRu4eC8jGoolKQ5hsqpIV5sG1NA4YB2HL4/edit?usp=sharing

This PR was earlier on #185

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is assigned to an appropriate reviewer.

@rt4914
Copy link
Contributor Author

rt4914 commented Oct 10, 2019

@veena14cs @nikitamarysolomanpvt

This PR is not yet finished, but it can be help you if you get stuck somewhere.

@rt4914
Copy link
Contributor Author

rt4914 commented Oct 10, 2019

@BenHenning @veena14cs @nikitamarysolomanpvt
This PR is ready for review.

@rt4914 rt4914 mentioned this pull request Oct 10, 2019
Closed
6 tasks
BenHenning
BenHenning requested changes Oct 11, 2019
View reviewed changes
Copy link
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.

Left some feedback about the overall approach. Will follow up with another deep dive once you have a chance to iterate on my comments.

app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated
// XX -> Numeric Input
val stateNumericInputAnswer = 121

when (resultEphemeralState.get()!!.state.interaction.id) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above--we need to generalize this. We should have a way to retrieve the current InteractionObject from the active interaction view, then pipe that to the controller.

app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt
private fun subscribeToAnswerOutcome(answerOutcomeResultLiveData: LiveData<AsyncResult<AnswerOutcome>>) {
val answerOutcomeLiveData = getAnswerOutcome(answerOutcomeResultLiveData)
answerOutcomeLiveData.observe(fragment, Observer<AnswerOutcome> {
explorationProgressController.moveToNextState()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? From the mocks it seems like some interactions submit upon selecting an answer (e.g. multiple choice), and others the 'submit' button changes to 'continue' once the answer is submitted. Either way, submitting the answer and progressing to the next card always seem to be distinct user actions (except for continue since there is no explicit answer submission).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check revised implementation TODO comment.

I have created one method onOptionSelected(isOptionSelected: Boolean), which will control the visibility of the submit-button.

But for now I have kept this true because, currently we don't have functionality of MultipleChoiceInput and ItemSelectionInput.

@BenHenning BenHenning assigned rt4914 and unassigned BenHenning Oct 11, 2019
@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Oct 12, 2019
@rt4914 rt4914 mentioned this pull request Oct 15, 2019
Closed
@nikitamarysolomanpvt
Copy link
Contributor

Assign once the new approach is implemented.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Oct 16, 2019
@rt4914
Copy link
Contributor Author

rt4914 commented Oct 18, 2019

This implementation has been changed and therefore closing this PR for now.

@rt4914 rt4914 closed this Oct 18, 2019
@rt4914 rt4914 mentioned this pull request Oct 21, 2019
Merged
6 tasks
@BenHenning BenHenning deleted the continue-end-button-low-fi-part-2 branch June 10, 2020 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

UI Structure: ContinueInteractionView [Blocked: #150]
4 participants
@rt4914 @nikitamarysolomanpvt @BenHenning @veena14cs