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 brackets and hyphen from phone number input #7328

Closed
wants to merge 15 commits into from
Closed

Remove brackets and hyphen from phone number input #7328

wants to merge 15 commits into from

Conversation

thesahindia
Copy link
Member

@thesahindia thesahindia commented Jan 19, 2022

Details

  • Created and added PhoneTextInput for fields that takes phone number
  • Removing parentheses and hyphens from the phone number
  • Updated the error message and used the same placeholder for all phoneInputs

Fixed Issues

$ #7007

Tests

  • Verify that no errors appear in the JS console

QA Steps

  1. Navigate to Concierge > Click on the Phone Icon
  2. Enter phone number in different formats (e.g with/without parentheses/dashes)
  3. Verify you don't get any error related to phone number

  1. Navigate to settings > Click on the Profile > Add Phone number
  2. Enter phone number in the phone input field with different formats (e.g with/without parentheses/dashes)
  3. Verify you don't get any error related to phone number

  1. Navigate to workspace > Connect bank Account > Connect manually > Fill the details and save
  2. Enter phone number in the company phone field with different formats (e.g with/without parentheses/dashes)
  3. Verify you don't get any error related to phone number

  1. Click on Fab icon > send money > onfido verification > Additional details page
  2. Enter phone number in Phone number field with different formats (e.g with/without parentheses/dashes)
  3. Verify you don't get any error related to phone number

  1. Go to search
  2. Search a phone number with parentheses and hyphen
  3. Verify that user shows up

  • Verify that no errors appear in the JS console

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2022-01-20.at.3.36.49.AM.mov
Screen.Recording.2022-01-20.at.3.34.10.AM.mp4

Mobile Web

Screen.Recording.2022-01-20.at.3.45.39.AM.mov
Screen.Recording.2022-01-20.at.3.51.16.AM.mov

Desktop

Screen.Recording.2022-01-20.at.4.07.37.AM.mov

iOS

Screen.Recording.2022-01-20.at.4.21.17.AM.mov
Screen.Recording.2022-01-20.at.4.16.15.AM.mov
Screen.Recording.2022-01-20.at.4.11.46.AM.mov

Android

Screen.Recording.2022-01-20.at.3.57.42.AM.mov
Screen.Recording.2022-01-20.at.4.00.40.AM.mov

@thesahindia thesahindia requested a review from a team as a code owner January 19, 2022 22:03
@MelvinBot MelvinBot requested review from parasharrajat and puneetlath and removed request for a team January 19, 2022 22:03
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/phoneTextnputPropTypes.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/CompanyStep.js Show resolved Hide resolved
src/pages/settings/AddSecondaryLoginPage.js Outdated Show resolved Hide resolved
src/pages/settings/AddSecondaryLoginPage.js Outdated Show resolved Hide resolved
@thesahindia
Copy link
Member Author

PR updated!
cc: @puneetlath @parasharrajat

@parasharrajat
Copy link
Member

Thanks, will test shortly.

src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/libs/ValidationUtils.js Outdated Show resolved Hide resolved
@thesahindia
Copy link
Member Author

PR is ready for review now.
cc: @parasharrajat @puneetlath

@puneetlath
Copy link
Contributor

@parasharrajat have you had a chance to review/test this?

@parasharrajat
Copy link
Member

Not yet. I was waiting for the query. I am yet to do the final review and test.

@thesahindia You have got some conflicts. thanks.

src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/libs/ValidationUtils.js Outdated Show resolved Hide resolved
src/pages/EnablePayments/AdditionalDetailsStep.js Outdated Show resolved Hide resolved
src/pages/EnablePayments/AdditionalDetailsStep.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/CompanyStep.js Outdated Show resolved Hide resolved
src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
@thesahindia
Copy link
Member Author

Made the suggested changes, it's ready for the review.
cc: @parasharrajat

src/components/PhoneTextInput/index.js Outdated Show resolved Hide resolved
src/libs/ValidationUtils.js Outdated Show resolved Hide resolved
src/libs/ValidationUtils.js Outdated Show resolved Hide resolved
src/pages/ReimbursementAccount/CompanyStep.js Outdated Show resolved Hide resolved
src/libs/LoginUtil.js Outdated Show resolved Hide resolved
src/pages/EnablePayments/AdditionalDetailsStep.js Outdated Show resolved Hide resolved
src/pages/RequestCallPage.js Outdated Show resolved Hide resolved
src/CONST/index.js Outdated Show resolved Hide resolved
src/libs/ValidationUtils.js Outdated Show resolved Hide resolved
Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

Does not look like this error message makes sense now on the Request a Call page. As we moved to a generic PhoneInput. Brackets and hyphens are allowed.

image

@puneetlath Should we update that as well?

Please update QA steps to show how to navigate to Navigate to RequestCallPage, AdditionalDetailsStep, CompanyStep, AddSecondaryLoginPage. Testers don't know about the components. Try to use the Page name.

Suggestion: Try to improve your commit messages for future PRs. They should briefly explain the change.
image

Copy link
Member

@parasharrajat parasharrajat left a comment

Choose a reason for hiding this comment

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

On the Company information page. There is length validation. This is different from other phone input in the app. Do we want to keep it like this or make same as other phone inputs?
cc: @puneetlath
image

@puneetlath
Copy link
Contributor

Hm, interesting. I would say make it the same as other phone inputs. That feels weird to me for phone numbers.

@parasharrajat
Copy link
Member

parasharrajat commented Mar 14, 2022

I don't mind it. But please ask this on slack for a general opinion explaining the motive behind the PR.

@thesahindia
Copy link
Member Author

Asked about it on slack, also at Add phone number page we show the error from the backend and we use the same component for adding email and the error message doesn't asks about removing the brackets or dashes, It says I couldn't validate the phone number, please try again with the country code (e.g. +15005550006). so I guess it's fine?

@parasharrajat
Copy link
Member

I would suggest asking this on the issue/slack thread. These decisions need to be made by the team.

@thesahindia
Copy link
Member Author

@parasharrajat, Pr is ready for the review now.

@parasharrajat
Copy link
Member

parasharrajat commented Apr 5, 2022

I'll review it asap. Prioritizing tasks. Delay caused due to broken Mac.

@marcaaron
Copy link
Contributor

What's happening with this PR it's been open for 4 months can someone give a brief summary please?

@thesahindia
Copy link
Member Author

The PR is about removing the brackets and hyphen from the phone number and for that we created a new component (phoneTextInput) and we added front-end validation for phone number at some places and we made some other code changes to keep the behaviour consistent of phone number fields. Basically the scope of this issue was increased

Also it took time because of some communication delays and also because of delays in code changes as I was working on other PRs.

@marcaaron marcaaron changed the title use PhoneTextInput Remove brackets and hyphen from phone number input Apr 7, 2022
src/pages/RequestCallPage.js Show resolved Hide resolved
src/components/OptionsSelector.js Show resolved Hide resolved
src/components/PhoneTextInput/index.js Show resolved Hide resolved
src/languages/en.js Show resolved Hide resolved
src/languages/es.js Show resolved Hide resolved
@parasharrajat
Copy link
Member

Please merge main as well.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

I'm not sure that I agree 100% with creating yet another component for this. I'd think that we should strip special chars for frontend validation and on form submission.

If we are going the new component route, we should make sure that it's compatible with the new Form component - docs. There's an example of a refactor to TextInput in this PR.

@parasharrajat
Copy link
Member

Yeah, Correct. @thesahindia you would have to make the new component compatible with Form Design.

@thesahindia
Copy link
Member Author

We are using TextInput which is already compatible with the new form so we won't need to worry about it right?

@parasharrajat
Copy link
Member

We are using TextInput which is already compatible with the new form so we won't need to worry about it right?

I think no. We have to update the new component as well. PhoneTextInput modifies the input value before posting it to the parent. So this modified value needs to be updated on the form.

I'm not sure that I agree 100% with creating yet another component for this. I'd think that we should strip special chars for frontend validation and on form submission.

we can also do this but I am sure how much effort it would be.

@luacmartins
Copy link
Contributor

There are a few callbacks that need to be updated for this component to the the interface that Form expects. You can follow the instructions given in this PR.

we can also do this but I am sure how much effort it would be.

Right now, it would involve updating each validate and submit method (~4 usages)?

@thesahindia
Copy link
Member Author

Right now, it would involve updating each validate and submit method (~4 usages)?

Honestly I prefer going this way instead of using a new component.

@thesahindia
Copy link
Member Author

Right now, it would involve updating each validate and submit method (~4 usages)?

@luacmartins, should we go this way?

@luacmartins
Copy link
Contributor

I'd say yes, but maybe we should check with the other reviewers as well @marcaaron @parasharrajat (Puneet is ooo)

@parasharrajat
Copy link
Member

I am fine with either of them.

@marcaaron
Copy link
Contributor

marcaaron commented Apr 20, 2022

Can someone quickly summarize what is blocking this PR please? Thanks!

Edit: On the original issue if possible.

@parasharrajat
Copy link
Member

any update @thesahindia?

@thesahindia
Copy link
Member Author

@parasharrajat, I was waiting for a reply from Marc but I think we can just move forward so I will just go with the new approach.

@marcaaron
Copy link
Contributor

@parasharrajat, I was waiting for a reply from Marc but I think we can just move forward so I will just go with the new approach.

What is the new approach? Please summarize it?

@thesahindia
Copy link
Member Author

Carlos suggested that instead of using a new component we should be stripping parentheses and dashes at validation and on form submission.

I'm not sure that I agree 100% with creating yet another component for this. I'd think that we should strip special chars for frontend validation and on form submission.

So we are thinking to move forward with it.

@thesahindia thesahindia closed this May 1, 2022
@marcaaron
Copy link
Contributor

@thesahindia Please summarize in a comment why this PR was closed.

@thesahindia
Copy link
Member Author

In this PR we were trying to create and use a new component for all the phone number fields, now that we are going with a different approach we won't need that component. I closed this PR as it was easy to raise a new one instead of reverting the changes.
cc: @marcaaron

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