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.

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 #161: Exploration player contentcard supports rich-text part. #275

Merged
merged 172 commits into from
Nov 5, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Oct 30, 2019

Explanation

This PR is duplicate of #245.
This PR is mainly to focus on exploration content card it contains implementation of html parser which parses HTML content.

Change Activity to ContentCardTestActivity in manifest to run the testcases.

BenHenning and others added 30 commits September 24, 2019 23:47
tests to verify correctness.

Also, added a method to facilitate notifying of DataProvider changes on
the UI thread.
… it up to

the data controller, and start adding tests.
Conflicts:
	domain/build.gradle
	domain/src/main/java/org/oppia/domain/exploration/ExplorationDataController.kt

Also, migrate the data controller to the retriever.
AnswerClassificationController, and attempted to make
ExplorationProgressController thread-safe.

The thread-safety led to significant interface changes in the progress
controller, and led to discovering some issues with the mediator live data
approach to interop coroutines and LiveData. This locking mechanism will
need to change since the optimal solution requires resolving #90.
with the current MediatorLiveData implementation (see #90 for more
context). Fix existing progress controller tests and add a few more. All
current progress controller tests are passing.
…ation

support for the second test exploration (about_oppia).
…-controller' into introduce-load-exploration-part-1
Separated this in its own PR.
@veena14cs veena14cs assigned rt4914 and unassigned veena14cs Nov 4, 2019
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, some nit changes.

@rt4914
Copy link
Contributor

rt4914 commented Nov 4, 2019

@veena14cs Once these final changes are done. Re-assign it to me.

@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 4, 2019
@veena14cs
Copy link
Contributor Author

@veena14cs Once these final changes are done. Re-assign it to me.

Changes are done, but I am not able to run test case. its giving below error
Screenshot 2019-11-04 at 2 52 01 PM

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

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 4, 2019
@veena14cs
Copy link
Contributor Author

@veena14cs Once these final changes are done. Re-assign it to me.

Changes are done, but I am not able to run test case. its giving below error
Screenshot 2019-11-04 at 2 52 01 PM

This error was thrown because of Android-studio update.

Copy link
Contributor

@jamesxu0 jamesxu0 left a comment

Choose a reason for hiding this comment

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

LGTM

@jamesxu0 jamesxu0 removed their assignment Nov 4, 2019
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! Just one comment.

@rt4914
Copy link
Contributor

rt4914 commented Nov 5, 2019

I am merging this PR on develop considering that we need this ASAP and this has already been approved.

@rt4914 rt4914 merged commit a50d6d8 into develop Nov 5, 2019
@seanlip seanlip deleted the conent-card-new branch November 6, 2019 02:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants