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

[wallet] UX fixes for native phone number #1717

Merged
merged 14 commits into from
Nov 18, 2019

Conversation

martinvol
Copy link
Contributor

@martinvol martinvol commented Nov 14, 2019

Description

  • Changed logging from error to info to prevent red screen when user dismiss the modal
  • Don't trigger the modal if there's text in the input
  • Differentiate between US and Canada phone numbers to show the right country.

Tested

  • Manually and added tests.

Other changes

Related issues

Backwards compatibility

jmrossy
jmrossy previously approved these changes Nov 14, 2019
Copy link
Contributor

@jmrossy jmrossy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set a more clear description for this before you merge it?

@martinvol martinvol changed the title [wallet] Changed logging [wallet] Changed logging to prevent error message when requesting user phone number Nov 14, 2019
@codecov
Copy link

codecov bot commented Nov 14, 2019

Codecov Report

Merging #1717 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1717   +/-   ##
=======================================
  Coverage   74.26%   74.26%           
=======================================
  Files         277      277           
  Lines        7617     7617           
  Branches      669      669           
=======================================
  Hits         5657     5657           
  Misses       1845     1845           
  Partials      115      115
Flag Coverage Δ
#mobile 74.26% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8429db...a189b4e. Read the comment docs.

@martinvol martinvol added the automerge Have PR merge automatically when checks pass label Nov 14, 2019
@jmrossy jmrossy removed the automerge Have PR merge automatically when checks pass label Nov 14, 2019
@jmrossy
Copy link
Contributor

jmrossy commented Nov 14, 2019

Removing automerge, see @annakaz 's comment

@martinvol martinvol force-pushed the martinvol/fix_error_native_picker branch from bcffaaf to 57c2b72 Compare November 14, 2019 18:19
@martinvol martinvol changed the title [wallet] Changed logging to prevent error message when requesting user phone number [wallet] UX fixes for native phone number Nov 15, 2019
@jmrossy jmrossy dismissed their stale review November 15, 2019 13:36

changes

@martinvol
Copy link
Contributor Author

Thanks to @jeanregisser I figured out a way to differentiate USA vs Canada and should be working fine now.

@martinvol martinvol requested review from jmrossy and annakaz November 15, 2019 15:31
packages/react-components/components/PhoneNumberInput.tsx Outdated Show resolved Hide resolved
} catch (error) {
console.info(
`${TAG}/triggerPhoneNumberRequestAndroid`,
'Could not request phone. This might be thrown if the user dismissed the modal',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you differentiate the case when the user dismissed the modal based on the error message thrown by the NativePicker?

Copy link
Contributor Author

@martinvol martinvol Nov 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately when the modal is dismissed SmsRetriever.requestPhoneNumber() throws a generic There was an error trying to get the phone number error :/

packages/react-components/components/PhoneNumberInput.tsx Outdated Show resolved Hide resolved
packages/react-components/components/PhoneNumberInput.tsx Outdated Show resolved Hide resolved
packages/react-components/components/PhoneNumberInput.tsx Outdated Show resolved Hide resolved
@martinvol martinvol requested a review from annakaz November 18, 2019 18:06
@martinvol
Copy link
Contributor Author

Should be good to go! @annakaz

@annakaz annakaz assigned annakaz and unassigned martinvol Nov 18, 2019
Copy link
Contributor

@annakaz annakaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor comments, but LGTM!

}

onChangeCountryQuery = (countryQuery: string) => {
ChangeCountryQuery = (countryQuery: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: changeCountryQuery no caps?

// linter doesn't allow following code without splitting this code in two lines
const number = await SmsRetriever.requestPhoneNumber()
return number
async _getPhoneNumberFromNativePickerAndroid() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't seen the _ used in other parts of our codebase, ok to just keep as getPhoneNumberFromNativePickerAndroid?

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