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 part #361: HTML formatting throughout app #404

Merged
merged 22 commits into from
Nov 25, 2019
Merged

Fix part #361: HTML formatting throughout app #404

merged 22 commits into from
Nov 25, 2019

Conversation

veena14cs
Copy link
Contributor

@veena14cs veena14cs commented Nov 18, 2019 •
edited

Explanation

This PR fixes Html Formatting in the following cases.

  • Images that are too large get cut off--we should resize these to take up the maximum space they have available without requiring scrolling, but maintain aspect ratio.
  • Newlines shouldn't be rendered.

@veena14cs veena14cs mentioned this pull request Nov 18, 2019
Merged
3 tasks
@veena14cs veena14cs changed the title Fix #361: HTML formatting throughout app Fix part #361: HTML formatting throughout app Nov 18, 2019
rt4914
rt4914 approved these changes Nov 18, 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, just nit changes.

utility/src/main/java/org/oppia/util/parser/HtmlParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned veena14cs and unassigned rt4914 Nov 18, 2019
BenHenning
BenHenning requested changes Nov 19, 2019
View reviewed changes
Copy link
Sponsor 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.

Can you please add tests for this new logic?

I'm also looking for some more context on this trimming behavior that's being added. It's not exactly clear to me how to solves the problem (in fact, adding tests could help clarify that since they should run the code through the usual/expected happy paths).

utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/parser/UrlImageParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/parser/HtmlParser.kt Outdated Show resolved Hide resolved
utility/src/main/java/org/oppia/util/parser/HtmlParser.kt Show resolved Hide resolved
@BenHenning BenHenning removed their assignment Nov 19, 2019
@nikitamarysolomanpvt
Copy link
Contributor

Iam unassigning me once the changes are implemented please assign me again.

@nikitamarysolomanpvt nikitamarysolomanpvt removed their assignment Nov 19, 2019
@BenHenning BenHenning mentioned this pull request Nov 20, 2019
Closed
@veena14cs veena14cs assigned rt4914 and BenHenning and unassigned veena14cs Nov 21, 2019
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Nov 25, 2019
@BenHenning BenHenning assigned veena14cs and unassigned BenHenning Nov 25, 2019
BenHenning
BenHenning approved these changes Nov 25, 2019
View reviewed changes
Copy link
Sponsor 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.

As discussed via chat, please remove the loops such that we’re only removing the first or last newline, rather than all first or last newlines. We can avoid the loops by updating the newline replace pattern added in #454 with one that replaces all variable newlines with one, eg: “\n{2,}” to solve the case of “\n\n\n” becoming “\n”.

veena14cs
veena14cs commented Nov 25, 2019
View reviewed changes
utility/src/main/java/org/oppia/util/parser/HtmlParser.kt

if (text.endsWith("\n")) {
text = text.substring(0, text.length - 1)
trimEnd += 2
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Workaround.

@veena14cs
Copy link
Contributor Author

“\n{2,}”

Changed loop to if statement in trimSpannable() method. I tried replacing “\n{2,}” it didnt workout for me I tried workaround on this

@veena14cs veena14cs merged commit e3ed84a into develop Nov 25, 2019
@veena14cs veena14cs deleted the fix-html-formatting branch November 25, 2019 12:06
@veena14cs
Copy link
Contributor Author

veena14cs commented Nov 25, 2019 •
edited

Fix part of #361.

@veena14cs veena14cs mentioned this pull request Nov 26, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@BenHenning BenHenning BenHenning approved these 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.

None yet
4 participants
@veena14cs @nikitamarysolomanpvt @rt4914 @BenHenning