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

TextBoxRegEx doesn't validate Phone Numbers in (###) ###-#### format properly #1821

Closed
michael-hawker opened this issue Feb 17, 2018 · 16 comments
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ extensions ⚡ in progress 🚧
Milestone

Comments

@michael-hawker
Copy link
Member

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  <!-- Please search GitHub for a similar issue or PR before submitting -->
[ ] Feature request <!-- Please file a UserVoice request and include the link below https://wpdev.uservoice.com/forums/110705-universal-windows-platform/category/193402-uwp-community-toolkit -->
[ ] Sample app request
[ ] Documentation issue or request
[ ] Question of Support request => Please do not submit support request here, instead see https://github.com/Microsoft/UWPCommunityToolkit/blob/master/contributing.md#question

Current behavior

Entering a phone number in the style (###) ###-#### is marked as invalid both of the sample apps phone number examples.

Expected behavior

Should be recognized as a valid phone number.

Minimal reproduction of the problem with instructions

  1. Open Sample App
  2. Go to Extensions -> TextBoxRegEx sample
  3. Enter phone number in either of first two textboxes in (###) ###-#### format

Note: it's marked as invalid.

Environment

Nuget Package(s): 

Package Version(s): 

Windows 10 Build Number:
- [ ] Anniversary Update (14393) 
- [ ] Creators Update (15063)
- [x] Fall Creators Update (16299)
- [ ] Insider Build (xxxxx)

App min and target version:
- [ ] Anniversary Update (14393) 
- [ ] Creators Update (15063)
- [x] Fall Creators Update (16299)
- [ ] Insider Build (xxxxx)

Device form factor:
- [x] Desktop
- [ ] Mobile
- [ ] Xbox
- [ ] Surface Hub
- [ ] IoT

@michael-hawker michael-hawker added bug 🐛 An unexpected issue that highlights incorrect behavior extensions ⚡ labels Feb 17, 2018
@michael-hawker michael-hawker added this to the v2.2 milestone Feb 17, 2018
@michael-hawker michael-hawker modified the milestones: v2.2, 3.0 Feb 23, 2018
@avknaidu
Copy link
Member

Here is a regex that i built.

(\([0-9]{3}\) ?)[0-9]{3}-[0-9]{4}|[0-9]{3}-?[0-9]{3}-?[0-9]{4}|([00|+][0-9]{1,2}) ?[0-9]{3}-?[0-9]{3}-?[0-9]{4}

Below is all formats that it would match.

image

here is a link to regexr.com

Let me know if this fits the requirement and I will do a PR.

@michael-hawker
Copy link
Member Author

michael-hawker commented Feb 23, 2018

Thanks @avknaidu, I think we should address this in 3.0, as I think it'll be hard to validate all the same scenarios from the current regex to the new regex and people's expectations.

We probably also want to have different modes for International vs US/Canada. This also seemed pretty comprehensive: http://phoneregex.com

Unfortunately it also looks like the C# port of libphonenumber is out of date.

@avknaidu
Copy link
Member

libphonenumber is updated just 8 hrs ago on nuget. are you saying it still does not handle all scenario's?

@michael-hawker
Copy link
Member Author

Nice @avknaidu, I think I must have found a dead fork then by mistake.

@nmetulev thoughts on pulling in this dependency for 3.0?

@windowstoolkitbot
Copy link

This issue seems inactive. Do you need help to complete this issue?

@nmetulev
Copy link
Contributor

If we can avoid adding another dependency, I think we should try.

@skendrot
Copy link
Contributor

I think a generic solution for phone numbers is fine and users can add their own regex for their needs

@windowstoolkitbot
Copy link

This issue seems inactive. Do you need help to complete this issue?

1 similar comment
@windowstoolkitbot
Copy link

This issue seems inactive. Do you need help to complete this issue?

@nmetulev
Copy link
Contributor

Agree with @skendrot, let's not add another dependency and instead try to solve this through a regex

@nmetulev nmetulev modified the milestones: 3.0, 3.1 May 23, 2018
@Bmartin2013
Copy link

Bmartin2013 commented Jun 26, 2018

By default, this control looks for a custom regular expression in order to check their inputs. In other words, if you don't force this control to validate phone numbers, you will be able to set your own format validation through a custom regular expression by declaring the Regex property in XAML file.

1821

For more information, you can check the control documentation: https://docs.microsoft.com/en-us/windows/uwpcommunitytoolkit/extensions/textboxregex

@Bmartin2013
Copy link

Bmartin2013 commented Jul 11, 2018

Since the refactor of the phone number method will be a breaking change and it's possible to work around this problem by setting a custom RegEx to check the input values @nmetulev can we close this issue?

@nmetulev
Copy link
Contributor

I don't think it's a breaking change as right now it doesn't work as expected, phone numbers should be recognized. We should update the regex here which should resolve the issue when ValidationType="PhoneNumber" is used.

@nmetulev nmetulev modified the milestones: 3.1, 4.0 Jul 11, 2018
@skendrot
Copy link
Contributor

@avknaidu Do you want to provide a PR with your regex?

@nmetulev nmetulev added this to the 4.0 milestone Jul 11, 2018
@avknaidu
Copy link
Member

@skendrot will do

@nmetulev nmetulev modified the milestones: 4.0, 4.1 Aug 10, 2018
@nmetulev nmetulev modified the milestones: 4.1, 5.0 Sep 26, 2018
@nmetulev
Copy link
Contributor

PR merged

@ghost ghost locked as resolved and limited conversation to collaborators Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior bugbash 🏗️ extensions ⚡ in progress 🚧
Projects
None yet
Development

No branches or pull requests

6 participants