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 #189

Closed
wants to merge 26 commits into from
Closed

Fix #161: Exploration player contentcard supports rich-text #189

wants to merge 26 commits into from

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Sep 26, 2019 •
edited
Loading

Explanation

This PR is replica of #125. It includes rich-text component that parses HTML content. This PR also extracts image from the HTML content.

@veena14cs veena14cs mentioned this pull request Sep 26, 2019
Closed
1 task
@veena14cs veena14cs changed the title Fix part of #161: Exploration player base 2 contentcard Fix #161: Exploration player base 2 contentcard Sep 26, 2019
@veena14cs veena14cs changed the title Fix #161: Exploration player base 2 contentcard Fix #161: Exploration player contentcard supports rich-text Sep 26, 2019
rt4914
rt4914 reviewed Sep 27, 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.

@veena14cs Thanks.
I have taken an overview review of this PR, finish these and later on I can take in-depth look at this PR.

app/src/main/java/org/oppia/app/fragment/FragmentComponent.kt Outdated Show resolved Hide resolved
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
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
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 and BenHenning Sep 27, 2019
@rt4914
Copy link
Contributor

rt4914 commented Sep 27, 2019

@veena14cs Please add test-cases for this PR

@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Sep 30, 2019
rt4914
rt4914 reviewed Sep 30, 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.

LGTM

@rt4914
Copy link
Contributor

rt4914 commented Sep 30, 2019

@BenHenning PTAL final review

@rt4914 rt4914 assigned BenHenning and unassigned rt4914 Sep 30, 2019
BenHenning
BenHenning requested changes Oct 1, 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 PR looks really solid, however I'd really like us to submit the image parser separately since I want to spend a bit more time looking at it in isolation, and seeing tests for it.

At a high-level, we should also be adding tests for all new UI components being introduced, we should ensure that we're using domain models instead of GAE models, and we should hook into domain controllers rather than faking out data in the UI (only fake data if there isn't a controller available, and please let me know right away if that happens since the infrastructure team is actively working to keep UI structure work unblocked).

Please let me know if you have any concerns or doubts with any of these. My intent is to ensure that there's as little work as possible beyond layouts and animations when finalizing these UIs later on. Ie, we should try to get the code structure itself as close to final as possible with these PRs.

app/build.gradle Outdated Show resolved Hide resolved
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
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
app/src/main/java/org/oppia/app/player/content/ContentListFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/layout/content_list_fragment.xml Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/data/UrlImageParser.kt Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentListFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/app/player/content/ContentListFragmentPresenter.kt Show resolved Hide resolved
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Oct 1, 2019
nikitamarysolomanpvt
nikitamarysolomanpvt approved these changes Oct 1, 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 @veena14cs Please add test-cases for this PR

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Oct 1, 2019
@veena14cs veena14cs changed the base branch from develop to exploration-player-1-base October 14, 2019 10:49
@veena14cs veena14cs changed the base branch from exploration-player-1-base to develop October 14, 2019 10:49
@veena14cs veena14cs mentioned this pull request Oct 14, 2019
Merged
@veena14cs
Copy link
Contributor Author

Duplicate of this PR is created here.

@veena14cs veena14cs closed this Oct 14, 2019
@veena14cs veena14cs deleted the exploration-player-2-contentcard-replica branch October 14, 2019 11:04
This was referenced Oct 14, 2019
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 left review comments

@BenHenning BenHenning BenHenning requested changes

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt approved these changes

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