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

[Hold] fixed spaces collapsing in Messages #3869

Merged
merged 4 commits into from
Jul 10, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jul 2, 2021

Details

#3859 (comment)

Fixed Issues

$ Fixes #3859

Tests

  1. Do npm install to get the latest packages used.
  2. Open E.cash local server.
  3. Go to any conversation and send the following message.
0 space
 1 spaces
  2 spaces
   3 spaces
    4 spaces

QA Steps

  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
  1. Output should contain the same amount of spacing as added.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image

Mobile Web

Screen Shot 2021-07-04 at 07 59 52

iOS

Android

Screenshot 2021-07-04 07:58:25

@parasharrajat parasharrajat marked this pull request as ready for review July 4, 2021 02:33
@parasharrajat parasharrajat requested a review from a team as a code owner July 4, 2021 02:33
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team July 4, 2021 02:34
@@ -54,10 +54,14 @@ const EXTRA_FONTS = [
*
* @param {number} contentWidth - The content width provided to the HTML
* component.
* @param {number} tagName - tagname
Copy link
Contributor

@jasperhuangg jasperhuangg Jul 6, 2021

Choose a reason for hiding this comment

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

Can we provide a better description for this parameter? @parasharrajat

Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Left a comment about some JSDoc stuff. Also, tested it and it doesn't seem to work, am I missing something? This doesn't seem to happen on main. Were there any changes in expensify-common that I'm missing?

2021-07-06_10-13-12.mp4

@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 6, 2021

did you run 'npm install'? @jasperhuangg

@jasperhuangg
Copy link
Contributor

@parasharrajat Ah forgot, make sure you include that in your testing instructions if there were changes to expensify-common.

@parasharrajat
Copy link
Member Author

@jasperhuangg Please, let me know if you are facing some issues while testing.

@jasperhuangg
Copy link
Contributor

@parasharrajat Still waiting on you to look into this.

@parasharrajat
Copy link
Member Author

Sorry @jasperhuangg, I thought you are still facing the issue so I waiting to get a response over it.
Anyways, I have updated it. Thanks.

jasperhuangg
jasperhuangg previously approved these changes Jul 9, 2021
Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

@parasharrajat Looks good! One small grammatical fix.

Co-authored-by: Jasper Huang <jasperhu@usc.edu>
@parasharrajat
Copy link
Member Author

@jasperhuangg Done. Thanks.

@parasharrajat parasharrajat changed the title fixed spaces collapsing in Messages [Hold] fixed spaces collapsing in Messages Jul 9, 2021
Copy link
Contributor

@jasperhuangg jasperhuangg left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for your patience!

@jasperhuangg jasperhuangg merged commit 2e5a7ee into Expensify:main Jul 10, 2021
@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.

@jasperhuangg
Copy link
Contributor

Ah crap, didn't see the HOLD on this. I opened up a revert #3969. Sorry for any inconvenience @parasharrajat!

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

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

@kavimuru
Copy link

Chat - Spaces are not displayed after sending the message

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

Actions 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

Platform:

iOS ✔️
mWeb ✔️
Android ✔️
Web ✔️

Build:

1.0.78-0

Notes/Images/Video:

Bug5152224_IMG_6279

Bug5152224_RPReplay_Final1626313159.mp4

@parasharrajat
Copy link
Member Author

This PR is reverted. So the original issue still exists.

@isagoico
Copy link

@jasperhuangg We're good to check off this PR from the list if the PR was reverted right? CC @roryabraham

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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

@parasharrajat parasharrajat deleted the space-collapsing branch November 20, 2023 13:07
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.

Spaces are not displayed after sending the message
5 participants