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

feat: AddressForm Component #128

Merged
merged 67 commits into from
Jul 31, 2018
Merged

feat: AddressForm Component #128

merged 67 commits into from
Jul 31, 2018

Conversation

nnnnat
Copy link
Contributor

@nnnnat nnnnat commented Jul 12, 2018

Resolves #121
Impact: minor
Type: feature

Component

Create an AddressForm component using reacto forms and preexisting form components

Validation
The Address Form component does a simple isRequired validation on all required fields. reacto-forms let you provide your own form validation as long as it's based on this spec this will allow users to add custom validation during implementation.

PhoneNumberInput
Included in this PR is a very MVP of a PhoneNumberInput component. Currently, it's just a wrapper around the TextInput that trims all non-digit chars from the entered string.
Example: (504) 393-7303 -> 5043937303

In the future, we'd like to have a more feature rich PhoneNumberInput that includes masking and real phone number validation.
Examples:
http://catamphetamine.github.io/react-phone-number-input/
https://nosir.github.io/cleave.js/

Screenshots

Empty
screen shot 2018-07-16 at 10 50 45 am

Editing
screen shot 2018-07-16 at 10 50 54 am

Mobile
screen shot 2018-07-16 at 10 51 11 am

Breaking changes

N/A

Notes

@cassytaylor & @rymorgan since this component was built inside the component library I've used the input components we built several months ago so they're based on these field designs

Testing

  1. Verify you can submit and edit an address
  2. Verify the new PhoneNumberInput will trim non-digit characters and only returns a formatted phone number (i.e: 504-123-1234 => 5041231234)

@nnnnat nnnnat self-assigned this Jul 12, 2018
@machikoyasuda
Copy link
Contributor

machikoyasuda commented Jul 13, 2018

🚀 Preview deployed

Built with commit 77032f4

https://deploy-preview-128--stoic-hodgkin-c0179e.netlify.com

@spencern spencern requested a review from a team July 17, 2018 20:13
@nnnnat
Copy link
Contributor Author

nnnnat commented Jul 26, 2018

@machikoyasuda & @kieckhafer this is ready for another review

Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

The issue I came across in the first review is resolved.

@spencern spencern dismissed aldeed’s stale review July 27, 2018 03:05

Changes made

Copy link

@rymorgan rymorgan left a comment

Choose a reason for hiding this comment

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

@nnnnat I have a question which is probably more related to the form fields vs the AddressForm. If there is formatting set for a phone number field can we format it in real time vs when you click out of a field?

```

#### Address Form Implementation Example
Simple `AddressForm` implementation example. Bind to the form element via a `ref` method that can be used my any `Button` to trigger `submit` & `validate` form methods.

Choose a reason for hiding this comment

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

Typo -- should say 'used by'

@cassytaylor
Copy link

@nnnnat this is looking good!
There are two "Address" fields (instead of Address and Address Line 2 (Optional)) - and in the mocks, the Phone number field is optional, too. Should "optional" fields be conveyed in the component?

@nnnnat
Copy link
Contributor Author

nnnnat commented Jul 27, 2018

@rymorgan Yes the "no special character" formatter runs after each new input and on blur, I'd like to be able to pass the phone number format style to the PhoneNumberInput somehow via props so we could change it easily. Do you have a format we could try to set as the default?

@cassytaylor I'll update that second address field label. I can make the phone field optional in the addressForm but I thought it was required now I might be making this up tho. As for having an "optional" convey down into the component I think that's a smart idea and I think the Field component would actually take care of it, it already has an isRequired prop that we use for the error/valid states we could also add a "(Optional)" at the end of each field label if isRequired=false.

@nnnnat
Copy link
Contributor Author

nnnnat commented Jul 27, 2018

@reactioncommerce/design I'm also trying to resolve #163 with this PR, I think I've got the correct font-size set for the iinputs/select

@rymorgan
Copy link

rymorgan commented Jul 27, 2018

@nnnnat I think we can default it to US and from a little research (XXX) XXX-XXXX is the dominant format. The caveat here is that the component should allow the user to easily override that formatting for UK/Europe etc. But, if I understand your component, it seems like that is the case so 👍

@rymorgan
Copy link

@nnnnat @cassytaylor - I think your idea of having a "'(Optional)' at the end of each field label if isRequired=false" is a good one for this specific component but probably doesn't work when you think about the operator side of things. Can that be something specific to this component? Or specific to storefront components?

@nnnnat
Copy link
Contributor Author

nnnnat commented Jul 27, 2018

@rymorgan we could make it an explicit prop on the Field component something like isOptional to trigger the "(Optional)" to be added to the label text.

@rymorgan
Copy link

@nnnnat yea, that makes sense to me.

@nnnnat
Copy link
Contributor Author

nnnnat commented Jul 31, 2018

@rymorgan I've created another issue for the phone number formatting that I can do in follow up PR. Was there anything else that I'm missing for this form or do you think I can merge?

Copy link

@rymorgan rymorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@nnnnat nnnnat merged commit 244d470 into master Jul 31, 2018
@nnnnat nnnnat deleted the feat-121-nnnnat-address-form branch July 31, 2018 16:26
@rc-publisher
Copy link
Collaborator

🎉 This PR is included in version 0.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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.

Address Form
8 participants