Skip to content

Render comments in list using same logic as comments details.#16000

Merged
khaykov merged 2 commits intotrunkfrom
issue/15882-update-comment-rendering-in-list
Mar 3, 2022
Merged

Render comments in list using same logic as comments details.#16000
khaykov merged 2 commits intotrunkfrom
issue/15882-update-comment-rendering-in-list

Conversation

@khaykov
Copy link
Copy Markdown
Contributor

@khaykov khaykov commented Feb 25, 2022

Fixes #15882

(cc @ParaskP7 )

This PR improves the way we render HTML content in Site Comment list, and removes reliance on TagSoup library in WpHtml.

I switched to using same rendering logic that is used for comment content, which means we are finally rendering mentions and images (including gifs), also HTML content, like quotes, is styled properly.

To test:

  • Open the site with a variety of comments (any active p2 will do) side by side in trunk and this branch and confirm that the things look better, and nothing is broken visually.

Regression Notes

  1. Potential unintended areas of impact
    Some HTML elements might render in unexpected way.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing.

  3. What automated tests I added (or what prevented me from doing so)
    No way to test html rendering at this time.

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link
Copy Markdown

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 25, 2022

You can test the changes on this Pull Request by downloading the APKs:

@khaykov khaykov requested a review from develric February 25, 2022 22:00
@ParaskP7 ParaskP7 self-assigned this Feb 28, 2022
@ParaskP7 ParaskP7 self-requested a review February 28, 2022 09:44
Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @khaykov !

Thank you so much for opening this PR and making the switch, this is awesome! 💯

This PR improves the way we render HTML content in Site Comment list, and removes reliance on TagSoup library in WpHtml.

I switched to using same rendering logic that is used for comment content, which means we are finally rendering mentions and images (including gifs), also HTML content, like quotes, is styled properly.

After reviewing and testing this PR I can verify that the rendering of comments looks actually better now than before. It is a 🟢 light from my side. This also means that we can now remove the whole WPHtml and HtmlToSpannedCoverter functionality altogether, which will allow us to free ourselves from the TagSoup library dependency. 🎉

I am going to request changes on removing the TagSoup library and all its references as part of this PR as well. You can follow this 8552141 commit as a reference on what needs and can be removed as part of this change. As a quick reference I am listing those changes for you:

  • implementation 'org.ccil.cowan.tagsoup:tagsoup:1.2.1' dependency.
  • void inject(HtmlToSpannedConverter object); Dagger injection.
  • WPHtml.java main class.
  • WPHtmlTest.java test class.
  • HtmlToSpannedCoverter main class.
  • 3 x media_image_placeholder.png drawable resources.

Once more, great work and thank you so much on following-up with #15882! 🥇

@peril-wordpress-mobile
Copy link
Copy Markdown

Warnings
⚠️ PR has more than 300 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

@khaykov
Copy link
Copy Markdown
Contributor Author

khaykov commented Mar 1, 2022

@ParaskP7 Thanks! Removed the code you mentioned 🚀

Copy link
Copy Markdown
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

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

👋 @khaykov !

@ParaskP7 Thanks! Removed the code you mentioned 🚀

This is awesome, thanks for all this clean-up work. I reviewed and tested it. It is a 🟢 light from my side! 🚀

@khaykov khaykov merged commit d59e97e into trunk Mar 3, 2022
@khaykov khaykov deleted the issue/15882-update-comment-rendering-in-list branch March 3, 2022 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lint - Remove Tag Soup Library and Html To Spanned Converter

2 participants