-
Notifications
You must be signed in to change notification settings - Fork 414
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
Phone verification #946
Phone verification #946
Changes from 8 commits
c0e9b92
c52c757
b9b7af2
4409315
247859a
fa7d265
faa2753
a34a61b
3ac913a
705a74e
99bdaa6
9e2d82e
e0de34d
bdc4969
23261d0
ca9f67b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import React from 'react'; | ||
import { connect } from 'react-redux'; | ||
import { doUserEmailNew, doUserPhoneNew, doUserInviteNew } from 'redux/actions/user'; | ||
import { | ||
selectEmailNewIsPending, | ||
selectEmailNewErrorMessage, | ||
selectPhoneNewErrorMessage, | ||
} from 'redux/selectors/user'; | ||
import UserFieldNew from './view'; | ||
|
||
const select = state => ({ | ||
isPending: selectEmailNewIsPending(state), | ||
emailErrorMessage: selectEmailNewErrorMessage(state), | ||
phoneErrorMessage: selectPhoneNewErrorMessage(state), | ||
}); | ||
|
||
const perform = dispatch => ({ | ||
addUserEmail: email => dispatch(doUserEmailNew(email)), | ||
addUserPhone: (phone, country_code) => dispatch(doUserPhoneNew(phone, country_code)), | ||
}); | ||
|
||
export default connect(select, perform)(UserFieldNew); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
import React from 'react'; | ||
import { Form, FormRow, Submit } from 'component/form.js'; | ||
|
||
class UserFieldNew extends React.PureComponent { | ||
constructor(props) { | ||
super(props); | ||
|
||
this.state = { | ||
phone: '', | ||
email: '', | ||
country_code: '+1', | ||
}; | ||
|
||
this.formatPhone = this.formatPhone.bind(this); | ||
} | ||
|
||
formatPhone(value) { | ||
const { country_code } = this.state; | ||
value = value.replace(/\D/g, ''); | ||
if (country_code === '+1') { | ||
if (!value) { | ||
return ''; | ||
} else if (value.length < 4) { | ||
return value; | ||
} else if (value.length < 7) { | ||
return `(${value.substring(0, 3)}) ${value.substring(3)}`; | ||
} | ||
const fullNumber = `(${value.substring(0, 3)}) ${value.substring(3, 6)}-${value.substring( | ||
6 | ||
)}`; | ||
return fullNumber.length <= 14 ? fullNumber : fullNumber.substring(0, 14); | ||
} | ||
return value; | ||
} | ||
|
||
formatCountryCode(value) { | ||
if (value) { | ||
return `+${value.replace(/\D/g, '')}`; | ||
} | ||
return '+1'; | ||
} | ||
|
||
handleChanged(event, fieldType) { | ||
const formatter = { | ||
email: _ => _, | ||
phone: this.formatPhone, | ||
country_code: this.formatCountryCode, | ||
}; | ||
this.setState({ | ||
[fieldType]: formatter[fieldType](event.target.value), | ||
}); | ||
} | ||
|
||
handleSubmit() { | ||
const { email, phone, country_code } = this.state; | ||
if (phone) { | ||
this.props.addUserPhone(phone.replace(/\D/g, ''), country_code.replace(/\D/g, '')); | ||
} else { | ||
this.props.addUserEmail(email); | ||
} | ||
} | ||
|
||
render() { | ||
const { cancelButton, emailErrorMessage, phoneErrorMessage, isPending, fieldType } = this.props; | ||
|
||
return fieldType === 'phone' ? ( | ||
<div> | ||
<p> | ||
{__( | ||
'Enter your phone number and we will send you a verification code. We will not share your phone number with third parties.' | ||
)} | ||
</p> | ||
<Form onSubmit={this.handleSubmit.bind(this)}> | ||
<FormRow | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this have to be collected separately as opposed to letting them type a full number? If it is separate field, could it at least be an inline select next to the full number entry? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose it could be a select. Twilio wants a separate country code. I tried to make this as fool proof and intuitive as possible. I think the way it is makes a lot of sense (it was based on a discussion in tech-app), but I am completely open to changing it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @liamcardenas yes, please make the country code a select displayed before the phone number entry. |
||
type="text" | ||
label="Country Code" | ||
name="country_code" | ||
value={this.state.country_code} | ||
errorMessage={phoneErrorMessage} | ||
onChange={event => { | ||
this.handleChanged(event, 'country_code'); | ||
}} | ||
/> | ||
<FormRow | ||
type="text" | ||
label="Phone" | ||
placeholder={this.state.country_code === '+1' ? '(555) 555-5555' : '5555555555'} | ||
name="phone" | ||
value={this.state.phone} | ||
errorMessage={phoneErrorMessage} | ||
onChange={event => { | ||
this.handleChanged(event, 'phone'); | ||
}} | ||
/> | ||
<div className="form-row-submit"> | ||
<Submit label="Submit" disabled={isPending} /> | ||
{cancelButton} | ||
</div> | ||
</Form> | ||
</div> | ||
) : ( | ||
<div> | ||
<p> | ||
{__("We'll let you know about LBRY updates, security issues, and great new content.")} | ||
</p> | ||
<p>{__("We'll never sell your email, and you can unsubscribe at any time.")}</p> | ||
<Form onSubmit={this.handleSubmit.bind(this)}> | ||
<FormRow | ||
type="text" | ||
label="Email" | ||
placeholder="[email protected]" | ||
name="email" | ||
value={this.state.email} | ||
errorMessage={emailErrorMessage} | ||
onChange={event => { | ||
this.handleChanged(event, 'email'); | ||
}} | ||
/> | ||
<div className="form-row-submit"> | ||
<Submit label="Submit" disabled={isPending} /> | ||
{cancelButton} | ||
</div> | ||
</Form> | ||
</div> | ||
); | ||
} | ||
} | ||
|
||
export default UserFieldNew; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
import React from 'react'; | ||
import { connect } from 'react-redux'; | ||
import { doUserEmailVerify, doUserPhoneVerify, doUserEmailVerifyFailure } from 'redux/actions/user'; | ||
import { | ||
selectEmailVerifyIsPending, | ||
selectEmailToVerify, | ||
selectPhoneToVerify, | ||
selectEmailVerifyErrorMessage, | ||
selectPhoneVerifyErrorMessage, | ||
selectUserCountryCode, | ||
} from 'redux/selectors/user'; | ||
import UserFieldVerify from './view'; | ||
|
||
const select = state => ({ | ||
isPending: selectEmailVerifyIsPending(state), | ||
email: selectEmailToVerify(state), | ||
phone: selectPhoneToVerify(state), | ||
countryCode: selectUserCountryCode(state), | ||
emailErrorMessage: selectEmailVerifyErrorMessage(state), | ||
phoneErrorMessage: selectPhoneVerifyErrorMessage(state), | ||
}); | ||
|
||
const perform = dispatch => ({ | ||
verifyUserEmail: (code, recaptcha) => dispatch(doUserEmailVerify(code, recaptcha)), | ||
verifyUserPhone: code => dispatch(doUserPhoneVerify(code)), | ||
verifyUserEmailFailure: error => dispatch(doUserEmailVerifyFailure(error)), | ||
}); | ||
|
||
export default connect(select, perform)(UserFieldVerify); |
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.
Is there a particular reason we use a single component to do two (seemingly) distinct duties, as opposed to
UserEmailNew
andUserPhoneNew
or equivalent?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.
this was in the spirit of DRY a la our coding maxims.
There is less of a case for userFieldNew to be used for two field types, but userFieldVerify is basically the same for both types. I made userFieldNew to be consistent with that.
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 don't see very much savings here aside from maybe some boilerplate in index.js, and it adds complexity to declaration and handling changes. I'd split these.