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

Manually apply multiple-single-input-interaction changes to develop [DO NOT MERGE] #266

Closed
wants to merge 19 commits into from
Closed

Manually apply multiple-single-input-interaction changes to develop [DO NOT MERGE] #266

wants to merge 19 commits into from

Conversation

BenHenning
Copy link
Member

Similar to #265, this is a version of #258 that's based on develop via content-card-from-develop. The process for creating this was similar to #265.

@BenHenning
Copy link
Member Author

/cc @veena14cs

@BenHenning BenHenning changed the base branch from develop to content-card-from-develop October 26, 2019 00:49
@BenHenning
Merge branch 'content-card-from-develop' into multiple-single-input-i…
f86eb60 
…nteraction-from-develop

Conflicts:
	app/src/main/java/org/oppia/app/player/state/StateAdapter.kt
	app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt
@BenHenning
Resolve unresolved conflicts.
940e3ec
@BenHenning BenHenning mentioned this pull request Oct 26, 2019
Closed
rt4914
rt4914 suggested changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

PTAL

Screenshot 2019-10-29 at 1 15 57 PM

This dummy button is visible which I think should not be there, you can make its visibility attribute gone

app/build.gradle Outdated Show resolved Hide resolved
app/build.gradle Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/audio/CellularDataDialogFragmentTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/audio/AudioFragmentTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentTest.kt Outdated Show resolved Hide resolved
app/src/main/res/values/ids.xml Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateSelectionInteractionTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateSelectionInteractionTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateSelectionInteractionTest.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateSelectionInteractionTest.kt Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Oct 29, 2019
@veena14cs veena14cs removed their assignment Oct 29, 2019
@rt4914
Copy link
Contributor

rt4914 commented Oct 29, 2019

PTAL

Screenshot 2019-10-29 at 1 15 57 PM

This dummy button is visible which I think should not be there, you can make its visibility attribute gone

@veena14cs can you please address this.

@veena14cs
Copy link
Contributor

PTAL
Screenshot 2019-10-29 at 1 15 57 PM
This dummy button is visible which I think should not be there, you can make its visibility attribute gone

@veena14cs can you please address this.

Resolved.

2 similar comments
@veena14cs
Copy link
Contributor

PTAL
Screenshot 2019-10-29 at 1 15 57 PM
This dummy button is visible which I think should not be there, you can make its visibility attribute gone

@veena14cs can you please address this.

Resolved.

@veena14cs
Copy link
Contributor

PTAL
Screenshot 2019-10-29 at 1 15 57 PM
This dummy button is visible which I think should not be there, you can make its visibility attribute gone

@veena14cs can you please address this.

Resolved.

@rt4914
Copy link
Contributor

rt4914 commented Oct 29, 2019

@veena14cs I think you should add test -cases related to configuration change too. Currently once the learner has selected some options and if you rotate the phone, then the selected options are not there. I think you should fix that and add configuration change test case accordingly.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Oct 29, 2019
@veena14cs
Copy link
Contributor

@veena14cs I think you should add test -cases related to configuration change too. Currently once the learner has selected some options and if you rotate the phone, then the selected options are not there. I think you should fix that and add configuration change test case accordingly.

Yes I am trying to figure it out, because there is list inside a list.

@BenHenning
Copy link
Member Author

Hi folks. Out of curiosity, why are we committing to this branch? :) This was meant to be an example of moving the multiple choice interaction to be based on develop, but it's not meant to actually go through a review.

@BenHenning BenHenning removed their assignment Oct 30, 2019
@nikitamarysolomanpvt
Copy link
Contributor

Still this issue is there

PTAL
Screenshot 2019-10-29 at 1 15 57 PM
This dummy button is visible which I think should not be there, you can make its visibility attribute gone

@veena14cs can you please address this.

Resolved.

@nikitamarysolomanpvt
Copy link
Contributor

nikitamarysolomanpvt commented Oct 30, 2019 •
edited
Loading

I have done a quick look on this review ,One of the test case is failing and some issues on ui are already addressed by rajat.

Hi folks. Out of curiosity, why are we committing to this branch? :) This was meant to be an example of moving the multiple choice interaction to be based on develop, but it's not meant to actually go through a review.

Iam unassigning and if you want me to review again please reassign

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Oct 30, 2019
@veena14cs
Copy link
Contributor

Hi folks. Out of curiosity, why are we committing to this branch? :) This was meant to be an example of moving the multiple choice interaction to be based on develop, but it's not meant to actually go through a review.

New PR is been created here #276 . It is upto date with develop but it depends on #205 and #275 .

@veena14cs veena14cs closed this Oct 30, 2019
@veena14cs veena14cs deleted the multiple-single-input-interaction-from-develop branch October 30, 2019 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veena14cs veena14cs veena14cs left review comments

@rt4914 rt4914 rt4914 requested changes

@nikitamarysolomanpvt nikitamarysolomanpvt Awaiting requested review from nikitamarysolomanpvt

Assignees

@veena14cs veena14cs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BenHenning @rt4914 @veena14cs @nikitamarysolomanpvt