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: login page error #5864

Merged
merged 5 commits into from
Oct 19, 2021
Merged

fix: login page error #5864

merged 5 commits into from
Oct 19, 2021

Conversation

parasharrajat
Copy link
Member

Details

Fixed Issues

$ #5821

Tests | QA Steps

  1. Open the Login page.
  2. Try the wrong phone number or without country code. You should get a relevant error.
  3. Try any text or wrong email. You should see a relevant error.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Mobile Web | Desktop | Android | IOS

output_file.mp4

@parasharrajat parasharrajat requested a review from a team as a code owner October 14, 2021 22:53
@MelvinBot MelvinBot requested review from mountiny and removed request for a team October 14, 2021 22:53
@parasharrajat parasharrajat changed the title fix: login page error [HOLD] fix: login page error Oct 14, 2021
Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Code looks good to me Rajat, great job. I will give it a test later as we will approach the hold to be lifted.

@mountiny mountiny changed the title [HOLD] fix: login page error fix: login page error Oct 19, 2021
@mountiny mountiny self-requested a review October 19, 2021 11:41
@parasharrajat
Copy link
Member Author

@mountiny This is ready.

@mountiny
Copy link
Contributor

@parasharrajat Thank you! i have tested for the US number with the dashes and the error then thinks it is an email:
image

I know this is one edge case, but do you think we could maybe add some other Regex, which would check for this type of phone number format (with parenthesis and dashes)? It might be helpful and it should not introduce too much complexity.

@parasharrajat
Copy link
Member Author

Ok. I still don't think this how it should be but I will add it.

@mountiny
Copy link
Contributor

mountiny commented Oct 19, 2021

@parasharrajat Feel free to discuss it before working on it, I am not sure if I understood correctly your previous comment about it. sorry for the confusion.

My point of view is, that simply from UX perspective, when user feeds something like this, they are clearly trying to add a phone number, but we will tell them it is a wrong email. I know it is not a big deal as it is edge case, but I think it is better UX if we catch this too (if we don't introduce some other regression by trying to do so of course).

@parasharrajat
Copy link
Member Author

Thanks for putting up a discussion. @mountiny

There are a couple of reasons, I don't feel that we should do this.

  1. No one is expected to put () in the login input. No other app handles this.
  2. If we are allowing the user to add this, then the input field should show the user the correct format.
  3. I don't feel having this as optional is worth it.

My suggestion is to improve error detection. as you can clearly see that this is a number. but the error says email.

I think we remove the () from the input value and then compare the error with the phone regex, they can show the user error about the phone number.

So
Either we handle this, and add the format e.g. an error message.
Or I handle the error correctly. I will remove the () from input and then compare with phone regex and if the match passed and input had (), we will still show the error. Why? it does not match our format.

If the match failed the input is the wrong email.

@mountiny
Copy link
Contributor

@parasharrajat

My suggestion is to improve error detection. as you can clearly see that this is a number. but the error says email.

I totally agree with this and that is what I am suggesting 😅 I am sorry if you have understood something else.

Let's make needed changes, so that if user adds phone number with () or -, we will show them the format of phone umber we expect and not message about wrong email.

@parasharrajat
Copy link
Member Author

I didn't see the half message after the image. Lols. Sorry.

@mountiny
Copy link
Contributor

Haha, no problem at all. Glad we understood each other now :)

@parasharrajat
Copy link
Member Author

@mountiny Changes pushed.

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

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

Tested and works well at least for the scenarios I could think of :)

Great job @parasharrajat

@mountiny mountiny merged commit 86d3ec9 into Expensify:main Oct 19, 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 by @mountiny in version: 1.1.8-10 🚀

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

@OSBotify
Copy link
Contributor

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

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.

3 participants