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

Validate phone number textellent #1035

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

Akhtam
Copy link
Member

@Akhtam Akhtam commented Apr 1, 2021

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

@@ -0,0 +1,26 @@

/**
* @module normalize/validatePhoneNumber
Copy link
Member

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.

Copy link
Member Author

@Akhtam Akhtam Apr 9, 2021

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. :)

Comment on lines 31 to 39
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);
}
Copy link
Member

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:

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?

Comment on lines +43 to +45
padding: 0.9375rem 0 0.9375rem 0.9375rem;
color: $color-grey6;
font-size: 1.0625rem;
Copy link
Member

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.

Comment on lines -32 to -33
height: 50px;
padding: 15px 20px;
Copy link
Member

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?

app/components/Texting/components/FormView/FormView.jsx Outdated Show resolved Hide resolved
<div className={styles.phoneInputBox}>
<span className={styles.phoneInputPrefix}>+1</span>
<input
type="text"
Copy link
Member

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 = {
Copy link
Member

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants