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

Update the HTML entity parsing #4859

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Aug 26, 2021

Details

  1. Now all the HTML entities are supported.

Fixed Issues

$ #4759

Tests | QA Steps

  1. open newDot on the web.
  2. Copy some text that contains whitespaces or other HTML entities from Google Docs or any other application.
  3. Paste that into the composer.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

WEB | Desktop

Screen.Recording.2021-08-27.at.9.30.01.AM.mov

@parasharrajat parasharrajat marked this pull request as ready for review August 26, 2021 23:28
@parasharrajat parasharrajat requested a review from a team as a code owner August 26, 2021 23:28
@MelvinBot MelvinBot requested review from tgolen and removed request for a team August 26, 2021 23:28
@parasharrajat parasharrajat changed the title Updare the HTML entity parsing Update the HTML entity parsing Aug 27, 2021
@parasharrajat
Copy link
Member Author

cc: @Beamanator as you already reviewed this PR on e-common.

Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

Note: The original post in slack (here) noted that newlines were stripped too -> should that be fixed in this issue or did you purposefully leave it out?

I can't record this behavior as...

Are you sure? :D What about something like this?

Screen.Recording.2021-08-27.at.9.30.01.AM.mov

@parasharrajat
Copy link
Member Author

Yeah. Thanks, I will add a video like this. I didn't think it through 🤭 .

@Beamanator
Copy link
Contributor

Haha feel free to steal my video for web testing (if you want)

@parasharrajat
Copy link
Member Author

Let me check the newline issue.

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 27, 2021

Ok. I looked at it. It seems p tag is used for newlines which I think is correct as P(paragraph) is a block element.

<meta charset="utf-8">
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"
    id="docs-internal-guid-7a434b3d-7fff-25be-ecb1-113f970fc0f6"><span
        style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">Test
        a big&nbsp;</span></p><br />
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span
        style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">Sdsad
        nowe lines</span></p>
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span
        style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">Sef</span>
</p>
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span
        style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">As</span>
</p>
<p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt;"><span
        style="font-size:11pt;font-family:Arial;color:#000000;background-color:transparent;font-weight:400;font-style:normal;font-variant:normal;text-decoration:none;vertical-align:baseline;white-space:pre;white-space:pre-wrap;">Dsa</span>
</p>

So this is not part of this PR or issue but I can add that if you want.

I think to solve this we should add a parsing rule for the p tag. I am sure that it will work great. But a good amount of testing is needed to make sure. I also found p's creating an issue while parsing from Mac Note.app.

let me know, what do you want me to do @Beamanator ?

@Beamanator
Copy link
Contributor

@parasharrajat thanks for researching that! I would agree that this "special entities" fix is different from "parsing P tags" so I think a separate issue & job would make sense, maybe even a discussion in #expensify-open-source about how we want to handle copy / pasting many empty lines (some apps keep all empty lines, some condense them to one)

@Beamanator Beamanator self-requested a review August 27, 2021 09:46
Copy link
Contributor

@Beamanator Beamanator left a comment

Choose a reason for hiding this comment

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

All you @tgolen

@parasharrajat
Copy link
Member Author

parasharrajat commented Aug 27, 2021

@Beamanator I think this is a small PR and Another set of eyes is probably not needed. So my suggestion is if you are happy with the changes we move to merge.

Saving time 💮

@Beamanator Beamanator merged commit 230c68a into Expensify:main Aug 27, 2021
@Beamanator
Copy link
Contributor

True, and @tgolen has lots of other shenanigans going on this week probs :D

@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Beamanator in version: 1.0.88-3 🚀

platform result
🤖 android 🤖 cancelled 🔪
🖥 desktop 🖥 cancelled 🔪
🍎 iOS 🍎 cancelled 🔪
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

OSBotify commented Sep 1, 2021

🚀 Deployed to production by @roryabraham in version: 1.0.90-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@Julesssss
Copy link
Contributor

Whoops, we missed the notification here as the issue wasn't exported. My fault.

@Julesssss
Copy link
Contributor

Will update the issue manually now.

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.

4 participants