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

Fix: Using trim() for better UX #4190

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

aman-atg
Copy link
Contributor

Details

Comment

Fixed Issues

#4155 and #4156

Tests || QA Steps

Search Page

  1. Search for an email
  2. Add extra space
  3. Search result should not disppear

Profile

  1. Add extra whitespace anywhere in either names
  2. Save button will not be activated

Emoji picker Menu

  1. Go to any chat
  2. Open emoji picker
  3. Search for an emoji
  4. Adding extra whitespace won't make the result(s) disappear

Tested On

  • [ ✔️ ] Web
  • [ ✔️ ] Mobile Web
  • [ ❌ ] Desktop
  • [ ❌ ] iOS
  • [ ✔️ ] Android

Screenshots

Web

Search

web

Profile

web2

Emoji Picker

web3

Mobile Web

Profile

mWeb

Search

mWeb2

Desktop

iOS

Android

Profile

mobile

Search

mobile2

@aman-atg aman-atg requested a review from a team as a code owner July 23, 2021 12:47
@MelvinBot MelvinBot requested review from johnmlee101 and removed request for a team July 23, 2021 12:47
@github-actions
Copy link
Contributor

github-actions bot commented Jul 23, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@aman-atg
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

Copy link
Contributor

@johnmlee101 johnmlee101 left a comment

Choose a reason for hiding this comment

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

Looks good!

@johnmlee101 johnmlee101 merged commit 1138300 into Expensify:main Jul 26, 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.

@OSBotify
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.80-3🚀

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

@aman-atg aman-atg deleted the aman-atg-fix-whitespace-issues branch July 28, 2021 21:38
@kavimuru
Copy link

Desktop - Profile - Save button always enabled and able to save with space in name

Expected Result:

Save button will not be activated

Actual Result:

Save button always enabled and able to save name with spaces

Actions Performed:

  1. Launch the app and login
  2. Click on avatar and select profile
  3. Add extra whitespace anywhere in either names

Platform:

iOS
Web ✔️
Desktop ✔️

Build:

1.0.81-0

Notes/Images/Video:

Bug5171703_Recording__995.mp4

@aman-atg
Copy link
Contributor Author

Web (v1.0.81-1)

image

Not observing this behaviour on staging (Web).

  • Maybe it's fixed for Desktop as well on v1.0.81 , I don't have a macbook right now else I would have shown. Can you please check for build v1.0.81-1 on Desktop

@isagoico
Copy link

isagoico commented Jul 29, 2021

I'm still able to reproduce this on Web/Chrome staging 1.0.81-1 \

image

@aman-atg
Copy link
Contributor Author

aman-atg commented Jul 29, 2021

@isagoico
Can you tell me if your name was already having extra whitespace from earlier versions?
Or did you change any other field like Preferred Pronouns or Timezone etc.

Still trying to figure out if there's any other reason.

New Changes

  • I found out that changing other fields in Profile Section will also result in saving names with extra whitespace so only disabling the Button won't completely fix this behaviour.

firstName,
lastName,

  • These two lines should be changed with the below code
            firstName: firstName.trim(),
            lastName: lastName.trim(),

@johnmlee101 Please review the proposed changes

@isagoico
Copy link

I just tried again in a new account (that didn't have a name set yet) and got the same behavior. I only edited the name and last name fields with white space and text. Hope this is helpful
image

@aman-atg
Copy link
Contributor Author

@isagoico Thanks for testing.

I only edited the name and last name fields with white space and text. Hope this is helpful

I think changing the text should enable the Button?

@isagoico
Copy link

So, let me see if I get this. The user is allowed to save the name that contains whitespace if the user changes the text? We thought it was a bug.

@aman-atg
Copy link
Contributor Author

So, let me see if I get this. The user is allowed to save the name that contains whitespace if the user changes the text? We thought it was a bug.

Yes, you're right it's a bug and the new proposed changes will hopefully fix it.

Earlier, it was thought that only disabling the Save Button will fix it.
But, as we can't stop user from adding a whitespace after changing the name or other fields, the best we can do is save the name after trimming it.

@isagoico
Copy link

Oh thanks a lot for the clarification 🙇‍♀️ we'll wait for the fixes so we can retest this then

@johnmlee101
Copy link
Contributor

@aman-atg are you working on a fix for this?

@aman-atg
Copy link
Contributor Author

New Changes

  • I found out that changing other fields in Profile Section will also result in saving names with extra whitespace so only disabling the Button won't completely fix this behaviour.

firstName,
lastName,

  • These two lines should be changed with the below code
            firstName: firstName.trim(),
            lastName: lastName.trim(),

@johnmlee101
I think these changes will fix the issue.

@johnmlee101
Copy link
Contributor

I think that makes sense! Can you spin up a PR and link both the original issue and this PR to it?

@aman-atg aman-atg mentioned this pull request Jul 30, 2021
5 tasks
Jag96 added a commit that referenced this pull request Jul 30, 2021
github-actions bot pushed a commit that referenced this pull request Jul 30, 2021
@isagoico
Copy link

Retested this and it was a pass 🎉
image

Checking it off.

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.81-4🚀

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