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 #387: Concept card linking #731

Closed
wants to merge 6 commits into from
Closed

Fix #387: Concept card linking #731

wants to merge 6 commits into from

Conversation

rt4914
Copy link
Contributor

@rt4914 rt4914 commented Mar 2, 2020 •
edited
Loading

Explanation

Fixes #387
This PR adds the concept card linking inside exploration player. This has been done by using #422
Thanks to @BenHenning for #422

TAG information (as confirmed with @seanlip ):

<oppia-noninteractive-skillreview skill_id-with-value="<[skillId]>" text-with-value="<[linkText]>"></oppia-noninteractive-skillreview>

How to test this:

Approach 1. Make HtmlParserTestActivity as your launcher activity.
Approach 2. Start Meaning of Equal Parts exploration. First answer is D. last option. In second state, answer 3/2 and you should see a refresher lesson text.

Pending Issues:

  • LiTagHandler is not getting used.

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 made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

rt4914
rt4914 commented Mar 2, 2020
View reviewed changes
utility/src/main/java/org/oppia/util/parser/HtmlParser.kt
val imageGetter = urlImageParserFactory.create(htmlContentTextView, entityType, entityId, imageCenterAlign)

val htmlSpannable = HtmlCompat.fromHtml(htmlContent, HtmlCompat.FROM_HTML_MODE_LEGACY, imageGetter, LiTagHandler()) as Spannable
val htmlSpannable = CustomHtmlContentHandler.fromHtml(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Earlier LiTagHandler was getting used to modify the bullets but now it is not getting used. Not sure how to introduce this back in the implementation. Need suggestions from @BenHenning

Copy link
Member

Choose a reason for hiding this comment

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

Could we create another custom tag handler for li tags? I'm wondering if we can leverage the existing tag handler machinery to also make the list tag adjustment work correctly.

@rt4914 rt4914 changed the title Fix #387: Concept card linking [WIP] Fix #387: Concept card linking Mar 2, 2020
@BenHenning
Copy link
Member

Thanks for doing this @rt4914! As general feedback, I suggest splitting this into two PRs: one for the custom tag handler machinery & dedicated tests, and a second for actually hooking it up to the concept card & tests for that. It's generally easier to reason about & verify correctness of code when you split apart the introduction of complex code from its integration points.

@rt4914
Copy link
Contributor Author

rt4914 commented Mar 3, 2020

@veena14cs @nikitamarysolomanpvt While reviewing this PR, do not focus on code because I will split this PR. But you can definelity check the implementation by the running the application.

@veena14cs
Copy link
Contributor

veena14cs commented Mar 3, 2020 •
edited
Loading

@veena14cs @nikitamarysolomanpvt While reviewing this PR, do not focus on code because I will split this PR. But you can definelity check the implementation by the running the application.

Okay. So far everything works properly. Assign me once it is ready for review.

@veena14cs veena14cs removed their assignment Mar 3, 2020
nikitamarysolomanpvt
nikitamarysolomanpvt reviewed Mar 3, 2020
View reviewed changes
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have done a review on html parser, Please assign me back after the implementation and changes suggested by ben is complete.

@BenHenning BenHenning marked this pull request as draft May 26, 2020 20:42
@oppiabot
Copy link

oppiabot bot commented Jun 15, 2020

Hi @rt4914. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks!

@BenHenning
Copy link
Member

@rt4914 is this ready for review? Also, were tests added (per the reason #422 was never sent for review)?

@BenHenning BenHenning mentioned this pull request Jun 23, 2020
Closed
@BenHenning BenHenning removed their assignment Jun 23, 2020
@rt4914
Copy link
Contributor Author

rt4914 commented Jul 9, 2020

Closing this PR as @BenHenning would be working on this issue and also this branch is out-of-date.

@rt4914 rt4914 closed this Jul 9, 2020
@BenHenning BenHenning mentioned this pull request Aug 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikitamarysolomanpvt nikitamarysolomanpvt nikitamarysolomanpvt left review comments

@BenHenning BenHenning Awaiting requested review from BenHenning

@veena14cs veena14cs Awaiting requested review from veena14cs

Assignees

@rt4914 rt4914

Labels
PR: don't merge - HAS MERGE CONFLICTS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support showing concept cards in the exploration player
4 participants
@rt4914 @BenHenning @veena14cs @nikitamarysolomanpvt