Add support for (XXX) XXX-XXXX format and ability to remove country code#61
Add support for (XXX) XXX-XXXX format and ability to remove country code#61wswoodruff wants to merge 12 commits intoPolymerElements:masterfrom
Conversation
add noCountryCode option
add parens formatting
fix validation
wswoodruff
changed the title
|
@WilliamSWoodruff Can you check the Travis please. Some tests are failing. @notwaldorf hey can we have something like this as soon as possible? |
|
@yordis Cool! I'll take a look at those failing tests |
|
@WilliamSWoodruff still failing in the test |
|
@yordis Ah, those tests were passing before I changed the default format to (xxx) xxx-xxxx. I'll fix them if you think it should be the default. Otherwise, should I put the default back to xxx-xxx-xxxx ? |
|
@WilliamSWoodruff put the default format back and if you can, please create a test that get your changes with the different format so we validates that works. |
|
@yordis @notwaldorf All tests are passing. Is this PR ready to go? |
| <span class="country-code">+[[countryCode]]</span> | ||
|
|
||
| <template is="dom-if" if="{{!noCountryCode}}"> | ||
| <span class="country-code">+[[countryCode]]</span> |
There was a problem hiding this comment.
I think using hidden is cheaper in this case than a dom-if
|
My biggest comment is that I am not convinced the bracketing is required. |
|
@notwaldorf These changes do affect the complexity of caret handling. The utility of the PR appeals mostly to US phone numbers, which is the default assumption. My argument would be a less-than-trivial performance hit to improve user experience. Parentheses and spaces act exactly the same if entered manually if they match the the phone-number-pattern |
|
@notwaldorf for US is actually common use parenthesis so from user experience this is something that add value. And I really need something like remove the Country Code, I don't want to show that in some interfaces. |
|
@WilliamSWoodruff check the error please |
|
@notwaldorf what do you think? |
| value = value.replace(/\)/g, ''); | ||
| value = value.replace(/\s/g, ''); | ||
|
|
||
| oldValue = oldValue.replace(/-/g, ''); |
There was a problem hiding this comment.
Why do we need to do this twice, for both value and oldValue?
| // when counting the position of new dashes in the pattern. | ||
| if (shouldFormat && i == (currentDashIndex - totalDashesAdded)) { | ||
|
|
||
| if (i == leftParenIndex) { |
There was a problem hiding this comment.
Can this be abstracted in a function?
There was a problem hiding this comment.
Can this be abstracted in a function?
|
@yordis I think this PR needs a lot of work. At the moment it's basically copy pasting the same code 3 times, which is very hard to read and maintain, and I'm a bit uncomfortable merging it as it is :( |
|
@notwaldorf I'd be happy to make those changes! |
|
Hey! Hi there! What happens finally with this feature @notwaldorf @yordis ?? It would be nice to me have it, because I need to support (XXX) XXX-XXXX format. Thanks! |
|
@drwaky no clue about it I tried to help on the process but @notwaldorf is the person that could help on this |
|
This PR is fairly old and there hasn't been much activity on it. |
|
@masonlouchart Unfortunately I don't work with Polymer anymore and I won't be able to finish this. You're welcome to pickup where I left off though |
|
This source code is old more than 2 years, there is conflict and a new version is released. Unfortunately it'd be wiser to drop it. |
|
Yes I think Polymer has bumped 3 major versions since this -- was written while working on v1 |
6 participants
No description provided.