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

Remove whitespace: pre because of extra empty lines bug #9195

Merged
merged 10 commits into from
Jul 15, 2022

Conversation

aldo-expensify
Copy link
Contributor

@aldo-expensify aldo-expensify commented May 26, 2022

Details

This PR needs the changes from the PR: https://github.com/Expensify/Web-Expensify/pull/33993 . This PR without PR#33993 shouldn't break anything.

This PR is reverting partially what was done here: https://github.com/Expensify/App/pull/9025/files

There are email clients like outlook that add a bunch of \n in between the html of the email. The whitespace: pre cause these \n to be rendered as empty lines.

This PR is applying whitespace: pre to messages that don't have source: "email" and whitespace: normal for messages that have it. Email clients don't use whitespace: pre when showing the email body, they use &nbsp to force/keep whitespaces.

When testing / QA we should make sure that the test / QA from these PRs also passes:

Fixed Issues

$ #8088

Tests

  1. Create two accounts, one using a free email from https://outlook.live.com/
  2. Log in into NewDot with one of these accounts and create a chat with the other, save the chat ID: http://localhost:8080/r/<chat ID>
  3. Send a comment with the following content (without codeblock):
    0 spaces
     1 spaces
      2 spaces
       3 spaces
        4 spaces
     
  4. Send another comment with the following content (including a code block)
    0 spaces
     1 spaces
      2 spaces
       3 spaces
        4 spaces
    
    ```
    some code
    ```
     
  5. You should see that whitespaces are respected and that there aren't extra white lines added
    image
  6. Send an email from the outlook email account so it gets converted as a message in the chat you just created. This SO is useful to get that going in DEV. You should be sending an email with:
    • To: "Expensify rID:<replace-chat-id-here>" <postmaster@sandboxbdd8eb86efaa4dd68b56e83b7b379bd9.mailgun.org>
    • Subject: What ever you configured (for example, in my case I use "ALDO_TESTING_CREATE_REPORT_COMMENT")
    • Body: type some message with some line jumps like this:
      image
  7. The email should appear in the new dot conversation
    image
  8. You should see that it doesn't have an excessive amount of empty lines

QA (Production)

  1. Create two accounts, one using a free email from https://outlook.live.com/
  2. Log in into NewDot with one of these accounts and create a chat with the other, save the chat ID: https://new.expensify.com/r/<chat ID>
  3. Send a comment with the following content (without codeblock):
    0 spaces
     1 spaces
      2 spaces
       3 spaces
        4 spaces
     
  4. Send another comment with the following content (including a code block)
    0 spaces
     1 spaces
      2 spaces
       3 spaces
        4 spaces
    
    ```
    some code
    ```
     
  5. You should see that whitespaces are respected and that there aren't extra white lines added
    image
  6. Send an email from the outlook email account so it gets converted as a message in the chat you just created. You should be sending an email with:
    • To: replies+<replace-your-chat-id-here>@expensify.com
    • Subject: Anything
    • Body: type some message with some line jumps like this:
      Screen Shot 2022-06-20 at 3 01 58 PM
  7. The email should appear in the new dot conversation
    image
  8. You should see that it doesn't have an excessive amount of empty lines
  • Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
    • I verified any copy / text shown in the product was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • Any functional components have the displayName property
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose and it is
  • If a new CSS style is added I verified that:
    • A similar style doesn’t already exist
    • The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.

PR Reviewer Checklist

  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • iOS / native
    • Android / native
    • iOS / Safari
    • Android / Chrome
    • MacOS / Chrome
    • MacOS / Desktop
  • I verified there are no console errors (if there’s a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained “why” the code was doing something instead of only explaining “what” the code was doing.
    • I verified any copy / text shown in the product was added in all src/languages/* files
    • I verified any copy / text that was added to the app is correct English and approved by marketing by tagging the marketing team on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named “index.js”. All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • Any functional components have the displayName property
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
    • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose and it is broken down into smaller components in order to separate concerns and functions
  • If a new CSS style is added I verified that:
    • A similar style doesn’t already exist
    • The style can’t be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • Verify that no errors appear in the JS console

Screenshots

Web

Before fixAfter fix
image image
image image

Mobile Web

Desktop

iOS

Android

Before fixAfter fix
image image
image image

@aldo-expensify aldo-expensify self-assigned this May 26, 2022
@aldo-expensify
Copy link
Contributor Author

@parasharrajat since you are somewhat familiarized with the problem... do you know of any regressions this PR could cause by removing whiteSpace: 'pre',?

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented May 26, 2022

Update: ❗ I think this is not avoid copy/pasting or not... is about a message containing a code block or not

I can see that this breaks in some cases when you copy/paste HTML stuff that had white spaces. I.e. copy the sample text from test step 3) and paste it as comment using CMD + SHIFT + V, whitespaces will be lost:

Screen Shot 2022-05-26 at 7 47 43 PM

The whitespaces are simply gone when looking at the DOM. The spaces seem to be getting lost in the rendering process because I can see them coming from the backend in the Report_History:

"0 spaces<br /> 1 spaces<br />  2 spaces<br />   3 spaces<br />    4 spaces<br /> <pre>some code</pre><pre>some more      code<br /><br />asdad asd asd. asda.       asd</pre><br />1. Some item<br />    i. Some subitem 1<br />    ii. Some subitem 2<br />2. Some item 2"

And this is one that looks fine:

"0 space<br />  2 spaces<br />    4 spaces<br />      6 spaces"

Screen Shot 2022-05-26 at 7 52 16 PM

I'm not sure about what is the difference yet.

Update: ❗ I think this is not avoid copy/pasting or not... is about a message containing a code block or not

@parasharrajat
Copy link
Member

I will dig this over the weekend and let you know. Thanks for waiting.

@aldo-expensify
Copy link
Contributor Author

The easiest way to reproduce is to add this line in the promise handler for DeprecatedAPI.Report_GetHistory here:

image

            data.history = [{"person":[{"type":"TEXT","style":"strong","text":"aldo+test-spacing1@expensifail.com"}],"actorEmail":"aldo+test-spacing1@expensifail.com","actorAccountID":11030556,"message":[{"type":"COMMENT","html":"     <div class=\"WordSection1\"> <p class=\"MsoNormal\">Hi SomeName, <\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <p class=\"MsoNormal\">Will tomorrow at 34 est. work?<\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <p class=\"MsoNormal\">Thanks,  Test<\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <\/div>xxxxxx xxxxxxxx xx xxx: xxxx x-xxxx xxx xxxxxxxxxx xx xxxxxxxxxxxxx xx xxxxxxxxxxxx xxxxx x.x. xxx, xx xxx xxxxxxx xxxxxxx xx xx xxxxxxxxx xx xxxxxxx x xxxxxxxxxx xxxxxxx xx xxxxxxx. xxx xxx xxxxxx xxx xx xxxxxxx xxxxxxxxxxx xxx xxxxxxxxxxx xxxxxxxx xxxx xxxxx  xxx (xxxxxx xxx xxx xxxxx.xxx xxxxxxx, xxxxx xxxxxx x-xxxx xxxxxxxxxxx xxxxxxx x xxxxxxxx xxxxxxx) xx xxxx x-xxxx xxxxxxx xx xxxxxxxxxx xxxx xxxxxxx xx xxxxxxxxxxxxxxxx@xxxxx.xxx. xx xxx xx xx, xxx xxxxxx xx xxxx xxxxxxx xxxx xx xxxxxxxx xxxxxxxx. xxx xxxxxxxxx  xxxxxx xxxxxxx xx 225 x xxxxxx xxxxx, xxxxx 2600, xxxxxxx, xx 60602-4903.  xxxx xxxxx xxxxxxx xx xxxx xxxxx xxx xx xxx xx xxx xxxxxxxxxxxx xxx xxx xxxxxxx xxxxxxxxxx xx xxxxxxxxxxxx xxxxxxxxxxx  xx xxxxx xxxxxxxxxxx xxxxxx xxxx xxxxxxxxxx xxxxx xxxxxxxxxx xxx. xx xxx xxx xxx xxx xxxxxxxx xxxxxxxxx, xxxxxx xxxxxx xxx xxxxxx xx xxxxx xxxxx xxxxxxxxxxx xxx xxxxxx xxxx xxxxxxx xxxxxxx xxxxxxx xxxxxxx xx xxxxxxxxxx xx xxxxxx. xxxx xxxxx xx xxx xxxxxxxx  xx xx x xxxxxxxx xx xxxxx xxxxxxx xxxxxxx xxxxxxxxxx, xxx xxx xxx xxxxxx xxxxxxxxx xx xxxx xxxxx xxxxxx xxx xx xxxxxxxxx xx x xxxxxx xxx xxxxxxx xxxxxx xxxxxxxxx xxxxxx. xxxxx xxx.xxxxx.xxx\/xxxxxxxxxx xxx xxxx xxxxxxxxxxx xxxxx xxxxx xxx xxx xxx xxxxxxxxxxxx.     ","text":"      Hi SomeName,  \u00a0 Will tomorrow at 34 est. work? \u00a0 Thanks,  Test \u00a0 xxxxxx xxxxxxxx xx xxx: xxxx x-xxxx xxx xxxxxxxxxx xx xxxxxxxxxxxxx xx xxxxxxxxxxxx xxxxx x.x. xxx, xx xxx xxxxxxx xxxxxxx xx xx xxxxxxxxx xx xxxxxxx x xxxxxxxxxx xxxxxxx xx xxxxxxx. xxx xxx xxxxxx xxx xx xxxxxxx xxxxxxxxxxx xxx xxxxxxxxxxx xxxxxxxx xxxx xxxxx  xxx (xxxxxx xxx xxx xxxxx.xxx xxxxxxx, xxxxx xxxxxx x-xxxx xxxxxxxxxxx xxxxxxx x xxxxxxxx xxxxxxx) xx xxxx x-xxxx xxxxxxx xx xxxxxxxxxx xxxx xxxxxxx xx xxxxxxxxxxxxxxxx@xxxxx.xxx. xx xxx xx xx, xxx xxxxxx xx xxxx xxxxxxx xxxx xx xxxxxxxx xxxxxxxx. xxx xxxxxxxxx  xxxxxx xxxxxxx xx 225 x xxxxxx xxxxx, xxxxx 2600, xxxxxxx, xx 60602-4903.  xxxx xxxxx xxxxxxx xx xxxx xxxxx xxx xx xxx xx xxx xxxxxxxxxxxx xxx xxx xxxxxxx xxxxxxxxxx xx xxxxxxxxxxxx xxxxxxxxxxx  xx xxxxx xxxxxxxxxxx xxxxxx xxxx xxxxxxxxxx xxxxx xxxxxxxxxx xxx. xx xxx xxx xxx xxx xxxxxxxx xxxxxxxxx, xxxxxx xxxxxx xxx xxxxxx xx xxxxx xxxxx xxxxxxxxxxx xxx xxxxxx xxxx xxxxxxx xxxxxxx xxxxxxx xxxxxxx xx xxxxxxxxxx xx xxxxxx. xxxx xxxxx xx xxx xxxxxxxx  xx xx x xxxxxxxx xx xxxxx xxxxxxx xxxxxxx xxxxxxxxxx, xxx xxx xxx xxxxxx xxxxxxxxx xx xxxx xxxxx xxxxxx xxx xx xxxxxxxxx xx x xxxxxx xxx xxxxxxx xxxxxx xxxxxxxxx xxxxxx. xxxxx xxx.xxxxx.xxx\/xxxxxxxxxx xxx xxxx xxxxxxxxxxx xxxxx xxxxx xxx xxx xxx xxxxxxxxxxxx.     ","isEdited":false}],"originalMessage":{"html":"     <div class=\"WordSection1\"> <p class=\"MsoNormal\">Hi SomeName, <\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <p class=\"MsoNormal\">Will tomorrow at 34 est. work?<\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <p class=\"MsoNormal\">Thanks,  Test<\/p><p><\/p> <p class=\"MsoNormal\"><\/p><p>\u00a0<\/p> <\/div>xxxxxx xxxxxxxx xx xxx: xxxx x-xxxx xxx xxxxxxxxxx xx xxxxxxxxxxxxx xx xxxxxxxxxxxx xxxxx x.x. xxx, xx xxx xxxxxxx xxxxxxx xx xx xxxxxxxxx xx xxxxxxx x xxxxxxxxxx xxxxxxx xx xxxxxxx. xxx xxx xxxxxx xxx xx xxxxxxx xxxxxxxxxxx xxx xxxxxxxxxxx xxxxxxxx xxxx xxxxx  xxx (xxxxxx xxx xxx xxxxx.xxx xxxxxxx, xxxxx xxxxxx x-xxxx xxxxxxxxxxx xxxxxxx x xxxxxxxx xxxxxxx) xx xxxx x-xxxx xxxxxxx xx xxxxxxxxxx xxxx xxxxxxx xx xxxxxxxxxxxxxxxx@xxxxx.xxx. xx xxx xx xx, xxx xxxxxx xx xxxx xxxxxxx xxxx xx xxxxxxxx xxxxxxxx. xxx xxxxxxxxx  xxxxxx xxxxxxx xx 225 x xxxxxx xxxxx, xxxxx 2600, xxxxxxx, xx 60602-4903.  xxxx xxxxx xxxxxxx xx xxxx xxxxx xxx xx xxx xx xxx xxxxxxxxxxxx xxx xxx xxxxxxx xxxxxxxxxx xx xxxxxxxxxxxx xxxxxxxxxxx  xx xxxxx xxxxxxxxxxx xxxxxx xxxx xxxxxxxxxx xxxxx xxxxxxxxxx xxx. xx xxx xxx xxx xxx xxxxxxxx xxxxxxxxx, xxxxxx xxxxxx xxx xxxxxx xx xxxxx xxxxx xxxxxxxxxxx xxx xxxxxx xxxx xxxxxxx xxxxxxx xxxxxxx xxxxxxx xx xxxxxxxxxx xx xxxxxx. xxxx xxxxx xx xxx xxxxxxxx  xx xx x xxxxxxxx xx xxxxx xxxxxxx xxxxxxx xxxxxxxxxx, xxx xxx xxx xxxxxx xxxxxxxxx xx xxxx xxxxx xxxxxx xxx xx xxxxxxxxx xx x xxxxxx xxx xxxxxxx xxxxxx xxxxxxxxx xxxxxx. xxxxx xxx.xxxxx.xxx\/xxxxxxxxxx xxx xxxx xxxxxxxxxxx xxxxx xxxxx xxx xxx xxx xxxxxxxxxxxx.     "},"avatar":"https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_2.png","created":"May 28 2022 9:10am PDT","timestamp":1653754210,"automatic":false,"sequenceNumber":3,"actionName":"ADDCOMMENT","shouldShow":true,"clientID":"","reportActionID":"63777"},{"person":[{"type":"TEXT","style":"strong","text":"aldo+test-spacing1@expensifail.com"}],"actorEmail":"aldo+test-spacing1@expensifail.com","actorAccountID":11030556,"message":[{"type":"COMMENT","html":"0 spaces<br \/> 1 spaces<br \/>  2 spaces<br \/>   3 spaces<br \/>    4 spaces<br \/> <pre>Test<\/pre>","text":"0 spaces  1 spaces   2 spaces    3 spaces     4 spaces  Test","isEdited":false}],"originalMessage":{"clientID":"1654303872951187","html":"0 spaces<br \/> 1 spaces<br \/>  2 spaces<br \/>   3 spaces<br \/>    4 spaces<br \/> <pre>Test<\/pre>"},"avatar":"https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_2.png","created":"Jun 3 2022 5:51pm PDT","timestamp":1654303873,"automatic":false,"sequenceNumber":2,"actionName":"ADDCOMMENT","shouldShow":true,"clientID":"1654303872951187","reportActionID":"63776"},{"person":[{"type":"TEXT","style":"strong","text":"aldo+test-spacing1@expensifail.com"}],"actorEmail":"aldo+test-spacing1@expensifail.com","actorAccountID":11030556,"message":[{"type":"COMMENT","html":"0 spaces<br \/> 1 spaces<br \/>  2 spaces<br \/>   3 spaces<br \/>    4 spaces","text":"0 spaces  1 spaces   2 spaces    3 spaces     4 spaces","isEdited":false}],"originalMessage":{"clientID":"1654303847866358","html":"0 spaces<br \/> 1 spaces<br \/>  2 spaces<br \/>   3 spaces<br \/>    4 spaces"},"avatar":"https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_2.png","created":"Jun 3 2022 5:50pm PDT","timestamp":1654303855,"automatic":false,"sequenceNumber":1,"actionName":"ADDCOMMENT","shouldShow":true,"clientID":"1654303847866358","reportActionID":"63775"},{"actionName":"CREATED","created":"Jun 3 2022 5:39pm PDT","timestamp":1654303194,"avatar":"https:\/\/d2k5nsl2zxldvw.cloudfront.net\/images\/avatars\/avatar_3.png","message":[{"type":"TEXT","style":"strong","text":"__FAKE__"},{"type":"TEXT","style":"normal","text":" created this report"}],"person":[{"type":"TEXT","style":"strong","text":"__FAKE__"}],"automatic":false,"sequenceNumber":0,"shouldShow":true}];

You should have the exact same messages I have.

Without the changes from this PR you should see:

image

With the change (remove whitespace:pre) from this PR you will see:

image

@parasharrajat
Copy link
Member

parasharrajat commented Jun 7, 2022

Sorry, I wasn't able to look into this on the weekend but I will follow up tomorrow. For now, I see that there is a issue with whitespace:pre as you mentioned. But I am shocked to see that whitespaces are preserved without it. Will debug this and let you know.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 8, 2022

Looking at the code, I can say that removing whitespace:pre is not completely safe. It will not preserve whitespaces in HTML. Your explanation is correct #8088 (comment).

What if we convert the &nbsp; to normal whitespace on client side? But again, where will we parse the message. It is directly comming from backend. So atlast, I would say best option would be prepare the message on backend.

When a user replies via email, parse the message and do the necessary conversion before saving.

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Jun 8, 2022 via email

@aldo-expensify
Copy link
Contributor Author

I gave it a try to converting the &nbsp; to normal spaces, but it looks the same. We still have empty divs taking space.

For the purpose of testing it quicker, I did it in the front here (ReportActionItemFragment):

image

I also tested adding enableExperimentalBRCollapsing and enableExperimentalGhostLinesPrevention to TRenderEngineProvider, but didn't help.

@parasharrajat
Copy link
Member

Got it. Then backend would be the only viable option.

@aldo-expensify
Copy link
Contributor Author

Got it. Then backend would be the only viable option.

Not necessarily. I'm somewhat confident in that email replies will very often contain &nbsp. We could check if they contain that, and if they do don't force the white-space: pre;

And by "backend would be the only viable option", what kind of processing would you suggest? because now we know that just replacing the &nbsp; by ' ' won't work

@parasharrajat
Copy link
Member

because now we know that just replacing the   by ' ' won't work

Yeah, it would be too much work. Normally Markup of emails is complex.

We could check if they contain that, and if they do don't force the white-space: pre;

Sounds like it will work. Remove the styles from baseFont and apply it to the wrapper.

A comment flag with source = 'email' should be displayed with
whitespace: 'normal';
@aldo-expensify aldo-expensify force-pushed the aldo_fix-empty-lines branch from 3a62b03 to c7e2d84 Compare June 9, 2022 20:31
@@ -103,7 +103,7 @@ const replaceNodes = (dom) => {
}

// Adding a new line after each comment here, because adding after each range is not working for chrome.
if (dom.attribs[tagAttribute] === 'comment') {
if (dom.attribs[tagAttribute] === 'comment' || dom.attribs[tagAttribute] === 'email-comment') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have a function like

function isCommentTag(nodeTag) {
    return nodeTag === 'email-comment' || nodeTag === 'comment';
}

@aldo-expensify aldo-expensify changed the title Remove whitespace: pre because of extra empty lines bug [HOLD] Remove whitespace: pre because of extra empty lines bug Jun 10, 2022
@aldo-expensify aldo-expensify changed the title [HOLD] Remove whitespace: pre because of extra empty lines bug [HOLD https://github.com/Expensify/Web-Expensify/pull/33993] Remove whitespace: pre because of extra empty lines bug Jun 10, 2022
@aldo-expensify aldo-expensify marked this pull request as ready for review June 20, 2022 22:29
@aldo-expensify aldo-expensify requested a review from a team as a code owner June 20, 2022 22:29
@aldo-expensify
Copy link
Contributor Author

cc @tgolen since you participated in this convo and in case you want to have a look

@melvin-bot melvin-bot bot requested review from Julesssss and removed request for a team June 20, 2022 22:34
@Julesssss
Copy link
Contributor

I'll try and remember to check this, but please remind me if I forget to review this once the other PR is merged.

@aldo-expensify
Copy link
Contributor Author

cc @roryabraham can you have a look a this too? since it is related to https://github.com/Expensify/Web-Expensify/pull/33993 😬

aldo-expensify and others added 2 commits June 28, 2022 14:21
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@aldo-expensify
Copy link
Contributor Author

Update: DRYer code

@aldo-expensify
Copy link
Contributor Author

I'll try and remember to check this, but please remind me if I forget to review this once the other PR is merged.

@Julesssss I merged the other PR, just letting you know in case you want to review this

Julesssss
Julesssss previously approved these changes Jun 29, 2022
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

Nice fix, looking good 👍

@aldo-expensify aldo-expensify changed the title [HOLD https://github.com/Expensify/Web-Expensify/pull/33993] Remove whitespace: pre because of extra empty lines bug Remove whitespace: pre because of extra empty lines bug Jul 6, 2022
@aldo-expensify
Copy link
Contributor Author

Removed hold for https://github.com/Expensify/Web-Expensify/pull/33993 (it has been deployed to production).. fixing conflicts now.

@aldo-expensify aldo-expensify dismissed stale reviews from Julesssss and roryabraham via 65fe23e July 6, 2022 22:01
@aldo-expensify
Copy link
Contributor Author

Update: had to resolve conflicts

@aldo-expensify
Copy link
Contributor Author

Bump, had to resolve conflicts, need an approval again to get this merged :)

Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

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

👍

@Julesssss Julesssss merged commit fc352dd into main Jul 15, 2022
@Julesssss Julesssss deleted the aldo_fix-empty-lines branch July 15, 2022 08:18
@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 production by @luacmartins in version: 1.1.85-8 🚀

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

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