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
32 changes: 30 additions & 2 deletions app/components/Texting/components/FormView/Form.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,38 @@
}
}

.phoneInputBox {
display: flex;
margin-top: 10px;
background-color: $color-white;
border: 1px solid #DDDDDD;
border-radius: 2px;
letter-spacing: 1px;
Akhtam marked this conversation as resolved.
Show resolved Hide resolved
&: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?

}

.phoneInputPrefix {
padding: 0.9375rem 0 0.9375rem 0.9375rem;
color: $color-grey6;
font-size: 1.0625rem;
Comment on lines +42 to +44
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.

}

.phoneInput {
border: none;
background: $color-white;
Akhtam marked this conversation as resolved.
Show resolved Hide resolved
padding-left: 5px;
margin: 0; /* to avoid Safari for adding extra margin */
Akhtam marked this conversation as resolved.
Show resolved Hide resolved
box-shadow: none !important;
Akhtam marked this conversation as resolved.
Show resolved Hide resolved
/* Remove Chrome's autocomplete background color for input */
&:-webkit-autofill {
-webkit-box-shadow: 0 0 0 100px white inset !important;
}
}

.input {
background-color: $color-white;
height: 50px;
padding: 15px 20px;
Comment on lines -32 to -33
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?

margin-top: 10px;
@media screen and (max-width: 900px ) {
height: 35px;
Expand Down
41 changes: 31 additions & 10 deletions app/components/Texting/components/FormView/FormView.jsx
Original file line number Diff line number Diff line change
@@ -1,20 +1,26 @@
import React, { useState } from 'react';
import PropTypes from 'prop-types';
import {
normalize,
validatePhoneNumber,
} from '../../../../utils/validatePhoneNumber';
import Heading from './Heading';
import Privacy from './Privacy';
import Buttons from './Buttons';
import styles from './Form.module.scss';

const initialState = {
recipientName: '',
phoneNumber: '',
agreed: false,
};

const FormView = ({ service, handleSubmit, closeModal }) => {
const [state, setState] = useState(initialState);
const { recipientName, phoneNumber, agreed } = state;
const { recipientName, agreed } = state;
const [phoneNumber, setPhoneNumber] = useState('');
const [isValidNumber, setIsValidNumber] = useState(false);
Akhtam marked this conversation as resolved.
Show resolved Hide resolved
const { serviceName, serviceId } = service;

const onChange = evt => {
const {
type,
Expand All @@ -26,6 +32,13 @@ const FormView = ({ service, handleSubmit, closeModal }) => {
setState(prevState => ({ ...prevState, [name]: newValue }));
};

const onPhoneNumberChange = evt => {
const normalized = normalize(evt.target.value);
const isValid = validatePhoneNumber(normalized);
setIsValidNumber(isValid);
setPhoneNumber(normalized);
};

const onSubmit = event => {
event.preventDefault();
const data = {
Expand Down Expand Up @@ -55,13 +68,17 @@ const FormView = ({ service, handleSubmit, closeModal }) => {
<div className={styles.dataRates}>
*Standard text and data rates may apply.
</div>
<input
type="text"
name="phoneNumber"
className={styles.input}
value={phoneNumber}
onChange={onChange}
/>
<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 😅.

name="phoneNumber"
className={styles.phoneInput}
value={phoneNumber}
maxLength="14"
onChange={onPhoneNumberChange}
/>
</div>
</label>
<label className={styles.checkBox}>
<input
Expand All @@ -74,7 +91,11 @@ const FormView = ({ service, handleSubmit, closeModal }) => {
I agree to receive text messages from SF Service Guide.
</label>
</div>
<Buttons disabled={!agreed} closeModal={closeModal} onSubmit={onSubmit} />
<Buttons
disabled={!agreed || !isValidNumber}
closeModal={closeModal}
onSubmit={onSubmit}
/>
<Privacy />
</div>
);
Expand Down
26 changes: 26 additions & 0 deletions app/utils/validatePhoneNumber.js
Original file line number Diff line number Diff line change
@@ -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. :)

* 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.

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.

/** @param {string} phoneNumber - phone number to normalize. */
normalize(phoneNumber) {
const numbers = phoneNumber.replace(/\D/g, '');
const char = { 0: '(', 3: ') ', 6: '-' };
let normalized = '';
for (let i = 0; i < numbers.length; i += 1) {
normalized += (char[i] || '') + numbers[i];
}
return normalized;
},
/** @param {string} phoneNumber - phone number to validate. */
validatePhoneNumber(phoneNumber) {
return PHONE_REGEX.test(phoneNumber);
},

};