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

Spaces are not displayed after sending the message #3859

Closed
isagoico opened this issue Jul 1, 2021 · 19 comments · Fixed by #3869
Closed

Spaces are not displayed after sending the message #3859

isagoico opened this issue Jul 1, 2021 · 19 comments · Fixed by #3869
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented Jul 1, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Log in to e.cash and navigate to a conversation
  2. Send the following text
0 space
 1 spaces
  2 spaces
   3 spaces
    4 spaces

Expected Result:

Spaces should be displayed in the message after it's sent

Actual Result:

No spaces are displayed in the conversation after sending the message

Workaround:

N/A

Platform:

Where is this issue occurring?

Web ✔️
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.75-5

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

image

Expensify/Expensify Issue URL:

View all open jobs on Upwork


From @mallenexpensify https://expensify.slack.com/archives/C01GTK53T8Q/p1625162491337400

Issue: Spaces at the beginning of lines are removed when sending a message

@isagoico isagoico added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @sophiepintoraetz (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jul 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@isagoico isagoico changed the title Spaces are removed after sending the message Spaces are nos displayed after sending the message Jul 1, 2021
@isagoico isagoico changed the title Spaces are nos displayed after sending the message Spaces are not displayed after sending the message Jul 1, 2021
@thienlnam thienlnam added the External Added to denote the issue can be worked on by a contributor label Jul 1, 2021
@MelvinBot
Copy link

Triggered auto assignment to @MitchExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@rdjuric
Copy link
Contributor

rdjuric commented Jul 1, 2021

I think this might also need a fix on the BE? Looks like we're doing a .trim() on the frontend and on the backend.
Sending reportComment: " hey you" to our API we receive html: "hey you" text: "hey you"

@sophiepintoraetz
Copy link
Contributor

Hmm - @isagoico / @thienlnam out of curiosity, why did we skip the triage process?

@parasharrajat
Copy link
Member

parasharrajat commented Jul 1, 2021

Proposal

This one is a bug with react-native-render-html but the lib owner has added an experimental flag that enables the whitespaces. Otherwise empty whitespaces will be collapsed. Due to the reason that

0 space
 1 spaces
  2 spaces
   3 spaces
    4 spaces

is converted to HTML 0 space<br /> 1 spaces<br /> 2 spaces<br /> 3 spaces<br /> 4 spaces.
It does not have enclosing tags which would have preserved the spaces. And the way, react-native-render-html handles whitespaces other whitespaces is being collapsed.

But setting dangerouslyDisableWhitespaceCollapsing to true keep those spaces.

Good News.

We were using the alpha version of this lib but the Lib owner released the Beta stable version today so we can upgrade and this flag will start working for us.

I have tested the code after upgrading the lib and there are no breaking changes. 🥳 🥳

Just need to change few props and the RenderHTML component will look like

  <HTML
            textSelectable={textSelectable}
            renderers={renderers}
            baseStyle={webViewStyles.baseFontStyle}
            tagsStyles={webViewStyles.tagStyles}
            enableCSSInlineProcessing={false}
            contentWidth={containerWidth}
            computeImagesMaxWidth={computeImagesMaxWidth}
            systemFonts={EXTRA_FONTS}
            dangerouslyDisableWhitespaceCollapsing
            renderersProps={{
                img: {
                    initialDimensions: {
                        width: MAX_IMG_DIMENSIONS,
                        height: MAX_IMG_DIMENSIONS,
                    },
                },
            }}
            source={{html}}
            debug={debug}
        />
    );

Result

image

@thienlnam
Copy link
Contributor

@sophiepintoraetz Ah my bad, I should have caught that - I didn't check the sequence of events and just went ahead with the process

@thienlnam
Copy link
Contributor

I think this might also need a fix on the BE? Looks like we're doing a .trim() on the frontend and on the backend.
Sending reportComment: " hey you" to our API we receive html: "hey you" text: "hey you"

For the example text, the spaces are within a text paragraph so they shouldn't be getting filtered out so this is probably something to do with our parser / Expensimark

@thienlnam
Copy link
Contributor

@parasharrajat 🟢 on the proposal, I don't think there is currently an upwork job for it yet but once that gets created we'll assign you to it

@thienlnam thienlnam assigned parasharrajat and unassigned thienlnam Jul 1, 2021
@MitchExpensify
Copy link
Contributor

Upwork job!

@MelvinBot
Copy link

Triggered auto assignment to @Beamanator (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@MitchExpensify MitchExpensify removed their assignment Jul 1, 2021
@thienlnam
Copy link
Contributor

@MitchExpensify Could you also please hire @parasharrajat for the job?

@MitchExpensify
Copy link
Contributor

Will do when @parasharrajat applies!

@isagoico
Copy link
Author

isagoico commented Jul 2, 2021

Hmm - @isagoico / @thienlnam out of curiosity, why did we skip the triage process?

@sophiepintoraetz / @thienlnam this was my bad, I created the issue with the AutoassignerTriage label accidentally (I pressed ctrl + enter which created the issue before I was done 🤦). We have been instructed that issues created on the #Expensify-open-source channel should have the engineering label from the start. My apologies for the confusion and the unnecessary bump 🙇‍♀️

@sophiepintoraetz
Copy link
Contributor

Thanks @isagoico for the context! @MitchExpensify - can you confirm this or did I miss something, as I thought all issues were to go through the triage > engineering flow?

(this might actually be tying into what John S is looking into)

@MitchExpensify
Copy link
Contributor

Yes thats right @sophiepintoraetz ! What issue is John working on?

@parasharrajat
Copy link
Member

Requesting to reopen this. Due to the reason PR was reverted after an accidental merge, we still have this issue.

I am holding this as it depends on another Issue

@parasharrajat
Copy link
Member

Hey Guys,#3869 PR was reverted as I wanted to hold that until #4047 is merged. I think he is more qualified to do the changes to upgrade which is a core part of my PR.
Also, there are very good points on why we should not move with this issue #3951 (comment) & #3951 (comment) from the Lib owner itself. After those points I agree, maybe it is best we don't do it. But #3869 is already fixing it if we want to proceed with this issue. Open to discussion.

@MitchExpensify
Copy link
Contributor

Hey @parasharrajat, I'd say this is a good discussion Slack channel if you'd like to bring it there! (Probably won't get many eyes on it here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants