-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Added Danish(DK) Phone number #2070
Conversation
Please add tests |
@staabm what do you mean ??? screenshots? |
I mean unit tests, see the tests folder. I also realized we dont have tests for the other languages, so this is not a must have. @Arkni could you add some example tests for phoneUs so people see a example? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. 2 small changes and we are good to land.
$.validator.addMethod( "phoneDK", function( phone_number, element ) { | ||
phone_number = phone_number.replace( /\s+/g, "" ); | ||
return this.optional( element ) || phone_number.length > 9 && | ||
phone_number.match( /^(\+45( )?)?([0-9]{2})((-| )?[0-9]{2})((-| )?[0-9]{2})((-| )?[0-9]{2})$/ ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use regex.test(string)
instead of string.match(regex)
. We only need to know whether the phone number matches the regex or not. We don't need to retrieve the matched parts.
Also, please use non-capturing instead of capturing groups [1]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay thanks, will look on it later this week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Arkni can this regex codestyle be enforced via jshint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None that I am aware of.
@staabm Tests should not block us from merging this PR though, as @HawkonDK can add them in another PR. |
@staabm jquery-validation/test/methods.js Lines 779 to 789 in 38cb9b9
|
@Arkni awesome, we should separate them into a „additional“ folder and equal named files like the impl then |
Hi @HawkonDK Thanks! |
Matches DK phone number format
allows '-' or ' ' as a separator and allows parens around area code
some people may want to put a '+45' in front of their number
but not