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 #161: Exploration player contentcard supports rich-text part [Blocked: #205] #245

Closed
wants to merge 37 commits into from
Closed

Fix #161: Exploration player contentcard supports rich-text part [Blocked: #205] #245

wants to merge 37 commits into from

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Oct 19, 2019 •
edited
Loading

Explanation

This PR depends on #205 which is under review and base off merge-fix which contains code of #205.
This PR is mainly to focus on exploration content card it contains implementation of html parser which parses HTML content. Even if the implementation of #205 changes it does not effect this PR.

Recyclerview Matcher file is added in this PR for testcase, It is already present in develop. Once #205 is approved and update to develop this file will be removed.

@veena14cs veena14cs requested a review from BenHenning October 19, 2019 07:07
@veena14cs veena14cs mentioned this pull request Oct 19, 2019
Closed
rt4914
rt4914 approved these changes Oct 19, 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.

Generally LGTM.

Do we need to introduce test cases in this PR?

app/src/main/java/org/oppia/app/player/state/StateAdapter.kt Outdated Show resolved Hide resolved
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 Outdated Show resolved Hide resolved
@rt4914 rt4914 changed the title Fix #161: Exploration player contentcard supports rich-text part Fix #161: Exploration player contentcard supports rich-text part (DO NOT MERGE) Oct 19, 2019
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Oct 19, 2019
@rt4914
Copy link
Contributor

rt4914 commented Oct 19, 2019

@veena14cs I suggest writing description related to the merge-fix branch and what does it contain and why are you using this, both of us has communicated regarding this but Ben and Nikita are not aware about this.

@veena14cs veena14cs requested a review from rt4914 October 19, 2019 10:16
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Oct 19, 2019
@veena14cs
Copy link
Contributor Author

Generally LGTM.

Do we need to introduce test cases in this PR?

Test case related to what actually.

BenHenning
BenHenning approved these changes Oct 21, 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.

Thanks @veena14cs. Why is this marked as do not merge? Is it just that it's blocked on #205? If so, please update the title to reflect it's blocked. "Do not merge" to me feels like it should never be merged, but in this case it's unclear.

I agree with @rt4914 that we should add a test case for this. Perhaps one test in StateFragment to verify it properly adds spannables to state with Oppia HTML content. That's expected behavior for StateFragment, and that test should fail if you removed the code being added here. That's then a worthwhile test since it's verifying the integration works.

Once that test is added, this change LGTM.

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

rt4914 commented Oct 21, 2019

Thanks @veena14cs. Why is this marked as do not merge? Is it just that it's blocked on #205? If so, please update the title to reflect it's blocked. "Do not merge" to me feels like it should never be merged, but in this case it's unclear.

I agree with @rt4914 that we should add a test case for this. Perhaps one test in StateFragment to verify it properly adds spannables to state with Oppia HTML content. That's expected behavior for StateFragment, and that test should fail if you removed the code being added here. That's then a worthwhile test since it's verifying the integration works.

Once that test is added, this change LGTM.

@BenHenning Actually, do not merge was added by me so that by mistake someone should not merge this because the merge-fix is a combination of different branches which have been not approved yet. I will check with @veena14cs on which one it is blocked an fix it accordingly.

@veena14cs veena14cs changed the title Fix #161: Exploration player contentcard supports rich-text part (DO NOT MERGE) Fix #161: Exploration player contentcard supports rich-text part [Blocked: #205] Oct 21, 2019
@rt4914 rt4914 removed their assignment Oct 21, 2019
BenHenning
BenHenning approved these changes Oct 25, 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.

Thanks @veena14cs! This LGTM!

app/src/main/java/org/oppia/app/application/ApplicationComponent.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/HtmlParserEntityTypeModule.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentContentCardTest.kt Show resolved Hide resolved
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Oct 25, 2019
@rt4914
Copy link
Contributor

rt4914 commented Oct 25, 2019

@veena14cs As per @BenHenning last comments I see that you were suppose to make entityType injectable and you have done that.

So this PR still LGTM.

De-assigning myself.

@rt4914 rt4914 removed their assignment Oct 25, 2019
nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Oct 25, 2019
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment

Choose a reason for hiding this comment

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

LGTM suggested some nit changes PTL

app/src/main/java/org/oppia/app/player/state/StateAdapter.kt Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/state/StateFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/app/player/state/StateFragmentContentCardTest.kt Outdated Show resolved Hide resolved
@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Oct 25, 2019
@BenHenning BenHenning mentioned this pull request Oct 26, 2019
Closed
BenHenning
BenHenning requested changes Oct 26, 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.

This branch is not merge compatible with develop. Please make this based off of develop before merging. I believe there are hidden changes in this branch that haven't yet been reviewed after cerateing #265.

@BenHenning BenHenning mentioned this pull request Oct 26, 2019
Closed
@veena14cs
Copy link
Contributor Author

Duplicate of this PR is create here #275 .

@veena14cs veena14cs closed this Oct 30, 2019
@veena14cs veena14cs mentioned this pull request Oct 30, 2019
Merged
@veena14cs veena14cs deleted the content-card branch October 30, 2019 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning requested changes

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

@rt4914 rt4914 Awaiting requested review from rt4914

Assignees

@veena14cs veena14cs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

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