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

Feature Request (Polish) : Format the phone numbers like 123-456-7890 #16099

Closed
kavimuru opened this issue Mar 18, 2023 · 29 comments
Closed

Feature Request (Polish) : Format the phone numbers like 123-456-7890 #16099

kavimuru opened this issue Mar 18, 2023 · 29 comments
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Reviewing Has a PR in review

Comments

@kavimuru
Copy link

kavimuru commented Mar 18, 2023

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


Problem:

Phone numbers are displayed without any format (123456789) which looks ugly

Solution:

We should better format them (e.g. 123-456-7890 instead of 1234567890). T

Context/Examples/Screenshots/Notes:

image (3)

Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @puneetlath
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1679077176883759

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013a52bba4f477d2f9
  • Upwork Job ID: 1638900508027600896
  • Last Price Increase: 2023-03-23
@kavimuru kavimuru added Weekly KSv2 NewFeature Something to build that is a new item. labels Mar 18, 2023
@melvin-bot melvin-bot bot locked and limited conversation to collaborators Mar 18, 2023
@puneetlath puneetlath self-assigned this Mar 19, 2023
@puneetlath
Copy link
Contributor

Ideally we can find a global solution to this. Such that phone numbers from any region will be formatted nicely according to that region. Perhaps we could use a JS library like this one to help here.

@Expensify Expensify unlocked this conversation Mar 20, 2023
@koko57
Copy link
Contributor

koko57 commented Mar 20, 2023

I'll be happy to work on this feature :)

@MelvinBot

This comment was marked as off-topic.

@koko57
Copy link
Contributor

koko57 commented Mar 21, 2023

Proposal

Please re-state the problem that we are trying to solve in this issue. What is the root cause of that problem?

The phone numbers displayed are not formatted well. They are just extracted from login or display name.

What changes do you think we should make in order to solve the problem?

We can use external library to format the numbers accordingly to the region. I think in our case we can use https://github.com/grantila/awesome-phonenumber - a lightweight library which uses Google's libphonenumber. It has a very simple API, decent docs and TS support. In App we have some utils dealing with phone numbers - we can make them use the lib.

What alternative solutions did you explore? (Optional)

We can use another library - libphonenumber-js.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Mar 21, 2023
@puneetlath puneetlath added the Weekly KSv2 label Mar 22, 2023
@puneetlath
Copy link
Contributor

That seems like a good option! I started a little bit of a discussion here about what considerations we should have when choosing a library.

In the meantime, let's assume we were to use that library. Let's talk a little bit about how we should display phone numbers. I would think something like:

  • Use the national format for any phone numbers from the same country as you
  • Use the international format for any phone numbers from other countries
  • Display phone numbers consistently wherever they are displayed

@koko57
Copy link
Contributor

koko57 commented Mar 23, 2023

@puneetlath I think I would go with the third option and display the numbers consistently using the international format. :) WhatsApp also follows this international format convention.

@puneetlath puneetlath added the Internal Requires API changes or must be handled by Expensify staff label Mar 23, 2023
@MelvinBot
Copy link

Job added to Upwork: https://www.upwork.com/jobs/~013a52bba4f477d2f9

@MelvinBot
Copy link

Triggered auto assignment to Contributor Plus for review of internal employee PR - @rushatgabhane (Internal)

@puneetlath
Copy link
Contributor

That library seems good to me. @rushatgabhane what do you think?

As for the display, I think using the international format is what is most commonly expected in Europe. But in the US, it is not typical to see the +1. So I think let's try doing 1 and 2 here. Where we display local phone number if in the same region and international phone number if viewing a number from a different region.

@koko57
Copy link
Contributor

koko57 commented Mar 24, 2023

@puneetlath @rushatgabhane so are we good to go with this one? 🙂

@puneetlath
Copy link
Contributor

Yes, let's go ahead.

@melvin-bot melvin-bot bot added the Overdue label Mar 27, 2023
@puneetlath puneetlath removed the Daily KSv2 label Mar 27, 2023
@puneetlath
Copy link
Contributor

Making weekly.

@melvin-bot melvin-bot bot removed the Overdue label Mar 27, 2023
@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 27, 2023

@koko57 @puneetlath yes, the library looks good to me as well

https://github.com/grantila/awesome-phonenumber#comparison-with-other-libraries

@koko57
Copy link
Contributor

koko57 commented Mar 30, 2023

@puneetlath should we also format the number here?

Screenshot 2023-03-30 at 19 17 26

Screenshot 2023-03-30 at 19 18 00

Should we make an exception here and display it with the country code prefix or should we use the national format as we agreed on?
Can I fix this issue with this unnecessary '@expensify.com' after the phone number as I'm working on it?

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 31, 2023

Display phone numbers consistently wherever they are displayed

@koko57 i think yes. we should make no exceptions for how we display phone numbers.

I think I understand why you think we should display country code for one time passwords.

But the chances of a person having two phone numbers, and them only differing by a country code is practically zero.

@rushatgabhane
Copy link
Member

rushatgabhane commented Mar 31, 2023

Can I fix this issue with this unnecessary '@expensify.com' after the phone number as I'm working on it?

I think we'll have to, right? Because we're passing it to the formatter. So let's do it!

@puneetlath
Copy link
Contributor

Agreed on both points!

@melvin-bot melvin-bot bot added the Reviewing Has a PR in review label Mar 31, 2023
@puneetlath
Copy link
Contributor

@koko57 how's this going? Close to being ready for review?

@koko57
Copy link
Contributor

koko57 commented Apr 5, 2023

yeah, I needed to do some additional changes, almost done, the only thing that left is the phone number in IOU Action messages #16804 (comment)

@koko57
Copy link
Contributor

koko57 commented Apr 5, 2023

ready for review 🙂

@puneetlath
Copy link
Contributor

Weird. Not sure why @rushatgabhane and I didn't get auto-assigned. Will review shortly!

@rushatgabhane
Copy link
Member

@puneetlath should I create a follow-up issue that allows searching users using formatted phone numbers? Pasting (301 123) 4567 gives no results.

image

A nice to have in that would be "as you type formatting" provided by the library. https://github.com/grantila/awesome-phonenumber#example-2

@puneetlath
Copy link
Contributor

@puneetlath should I create a follow-up issue that allows searching users using formatted phone numbers? Pasting (301 123) 4567 gives no results.

Yes, definitely. I thought that was actually supposed to work already, so if it isn't, let's fix it.

@rushatgabhane
Copy link
Member

rushatgabhane commented Apr 19, 2023

Note to self :

  • Create follow up PR to add Str function for hard spaces
  • Create follow up issue for using Display name even if login is phone number

@puneetlath
Copy link
Contributor

Create follow up issue for using Display name even if login is phone number

@rushatgabhane I'll create the follow up issue for this actually if you don't mind. I've discovered a couple of things while implementing back-end changes.

@rushatgabhane
Copy link
Member

@puneetlath sure, that'd be great

@melvin-bot
Copy link

melvin-bot bot commented May 12, 2023

This issue has not been updated in over 15 days. @puneetlath, @koko57, @rushatgabhane eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

@puneetlath
Copy link
Contributor

Whoops. Need to pay out Rushat and then this can be closed.

@puneetlath
Copy link
Contributor

All paid. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Monthly KSv2 NewFeature Something to build that is a new item. Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

5 participants