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

Delivery: Save volunteer phone number #174

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ifearcompilererrors
Copy link
Collaborator

@ifearcompilererrors ifearcompilererrors commented Nov 10, 2020

issue #156

How to test

  • Visit delivery page
  • Claim a delivery
  • Enter your phone number. If you have autofill enabled on your browser, the autofill dropdown should appear
  • Click Submit

@ifearcompilererrors ifearcompilererrors changed the title [wip] Delivery: Save volunteer phone number Delivery: Save volunteer phone number Nov 10, 2020
@ifearcompilererrors ifearcompilererrors marked this pull request as ready for review November 10, 2020 02:37
@@ -221,7 +221,6 @@ describe("findDeliveryNeededRequests", () => {
});

describe("when an error occurs", () => {
let result;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@sandfort added you as a reviewer here since i believe you're helping out with airtable test suites

Copy link
Collaborator

@piratefsh piratefsh left a comment

Choose a reason for hiding this comment

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

Yay thanks for working on this!! Code looks good to me, I just have a couple of questions:

  • I'm a bit worried that the placeholder phone number can be confusing. I'm not up to date with UI best practices though, any thoughts on whether its more helpful to have a placeholder vs blank?
  • Just curious, did we have to disable country code to get autofill to work?

@ifearcompilererrors
Copy link
Collaborator Author

ifearcompilererrors commented Nov 16, 2020

Yay thanks for working on this!! Code looks good to me, I just have a couple of questions:

Thank you for taking a look!

  • I'm a bit worried that the placeholder phone number can be confusing. I'm not up to date with UI best practices though, any thoughts on whether its more helpful to have a placeholder vs blank?

I can see how this is confusing- let me see what I can adjust based on this https://medium.com/nextux/form-design-best-practices-9525c321d759

  • Just curious, did we have to disable country code to get autofill to work?

Yes, MuiPhoneNumber pre-populated the input value with the country code, and since we discard the country code prefix anyway i disabled it. The input value needs to be blank for autofill to kick in

Base automatically changed from master to main February 10, 2021 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants