-
Notifications
You must be signed in to change notification settings - Fork 22
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
Validate phone number textellent #1035
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,26 @@ | |||
|
|||
/** | |||
* @module normalize/validatePhoneNumber |
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.
Forgive me if I'm wrong, but did you copy this file from somewhere else? This doc comment @module
declaration doesn't match the actual file path, and we don't even use @module
declarations elsewhere in the codebase. In addition, the style of this file is completely unlike the style of any of the other code in this codebase, which is why I wanted to make sure I ask.
If you did copy this from elsewhere, then 1) we really need to provide attribution for it, both for legal reasons and just for ethics, and 2) we should really check the license of the code to make sure we are compliant (most licenses at least ask you to provide attribution, but they may have stronger requirements). Also, although it's less of an issue in our codebase, since we decided to give AskDarcel one of the stricter copyleft licenses, GPLv3, but in general, you should be very careful about code that you copy from elsewhere, since the license of that code may force you to relicense your own code that you're integrating it with.
If you didn't copy this, then disregard everything that I wrote above, and I apologize for falsely suspecting that this code may have been copied.
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.
ohh I tried to write some documentation but didn't realize I did it wrong but I did copy Regex's from stackoverflow. :)
display: flex; | ||
margin-top: 10px; | ||
background-color: $color-white; | ||
border: 1px solid #DDDDDD; | ||
border-radius: 2px; | ||
letter-spacing: 1px; | ||
&:active, &:hover, &:focus { | ||
box-shadow: 0 2px 4px 0 rgba(0,0,0,0.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.
It seems that most of these styles are copied from the input
style in _forms.scss:
askdarcel-web/app/styles/st-base/_forms.scss
Lines 11 to 26 in 7e4103e
input, textarea { | |
@extend %font-size-default; | |
padding: calc-em(15px); | |
width: 100%; | |
border: 1px solid #DDDDDD; | |
border-radius: 2px; | |
background-color: #F2F2F2; | |
font-size: calc-em(17px); | |
color: #333333; | |
&:active, &:hover, &:focus { | |
box-shadow: 0px 2px 4px 0px rgba(0,0,0,0.1); | |
} | |
&:disabled { | |
box-shadow: none; | |
} | |
} |
I am assuming that you have done this because .phoneInputBox
is not actually an input element, but a plain div
that houses both the prefix and the actual input element. Could you extract those styles into a mixin so that we ensure that the styles stay in sync between the generic input elements and these ones?
padding: 0.9375rem 0 0.9375rem 0.9375rem; | ||
color: $color-grey6; | ||
font-size: 1.0625rem; |
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.
These are pretty odd numbers to be setting. If you can extract the styles from the common form styles, then you won't may not need these, but if you do set them here, you should probably be using the calc-em()
function if you're trying to set sizes in relatively-sized units like rem
.
height: 50px; | ||
padding: 15px 20px; |
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.
What's the reason you decided to get rid of this?
<div className={styles.phoneInputBox}> | ||
<span className={styles.phoneInputPrefix}>+1</span> | ||
<input | ||
type="text" |
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.
I know that the previous version of this code didn't do this either, but could you change the type to tel
? This is a special input element type that mostly looks like a normal text box, but on most mobile devices will cause a numeric keypad to pop open instead of a full text keyboard, which makes it easier to enter numbers.
Also, now that I'm looking at the docs for the tel
input type, I'm seeing that it actually has a builtin pattern
attribute for specifying a regex for validating the field, so maybe we should be using that instead of building our own custom JavaScript logic for it? Sorry for not remembering about that attribute beforehand 😅.
// eslint-disable-next-line import/no-unused-modules | ||
const PHONE_REGEX = /^\(?(\d{3})\)?[- ]?(\d{3})[- ]?(\d{4})$/; | ||
|
||
module.exports = { |
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 ES6-style exports (i.e. the export
keyword) instead of CommonJS exports (i.e. module.exports
). Our app is not a Node.js application, so we don't need compatibility with that ecosystem, and even if we ever did need to, because we run our app through Webpack anyway, we could always ask Webpack to output a Node.js-compatible bundle, even if we use ES6 exports internally.
* Validator for US only phone numbers. | ||
*/ | ||
|
||
// eslint-disable-next-line import/no-unused-modules |
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 don't silence this lint rule. It's put there to prevent us from leaving around any code that is unused. If it goes off, it means that any publicly exported name isn't actually being used anywhere else in the app.
Added Normalize phone number for input: (111) 111-1111
Added Validation for US only phone numbers, keep Submit button disabled until phone number is valid.
make sure to set
const isProduction = true
to false inside SearchResults Component to display textellent feature