Convert AddressForm From Angular to React #969
Conversation
reldredge71
force-pushed
the
payment-methods-react
branch
from
21ec673 to
bef36b7
Compare
reldredge71
force-pushed
the
payment-methods-react
branch
from
bef36b7 to
1bf6987
Compare
wrandall22
left a comment
wrandall22
left a comment
There was a problem hiding this comment.
Looks like translations are still to be figured out
| <span>{error}</span> | ||
| { retry && ( | ||
| <button | ||
| id="retryButton" |
There was a problem hiding this comment.
How does this work when there are multiples on the same page? Won't the ids clash? This might need to be overwrite-able.
There was a problem hiding this comment.
Good point, this might have to be more dynamic to account for multiple instances.
| value, | ||
| error, | ||
| }: TextInputProps) => ( | ||
| <div className={`form-group${required && ' is-required' || ''}${error && ' has-error' || ''}`}> |
There was a problem hiding this comment.
This structure isn't always in place for text inputs. What's the best way to build a different structure when we need it? (e.g. personalOptionsModal.tpl.html)
There was a problem hiding this comment.
I could maybe remove this wrapping div from here, and create a wrapper component that will only be used for AddressForm text inputs.
| { title && <label>{title}</label> } | ||
| <input | ||
| type={type} | ||
| className="form-control form-control-subtle" |
There was a problem hiding this comment.
Are these overridable? Not all text inputs have form-control-subtle, for example.
There was a problem hiding this comment.
Yes, I could create an optional prop that overrides the input className value.
| <CountrySelect | ||
| addressDisabled={addressDisabled} | ||
| countries={countries.map(country => ({ name: country.name, displayName: country['display-name']}))} | ||
| onChange={handleChange} |
There was a problem hiding this comment.
This should call refreshRegions I think. Not seeing how that is being done here.
There was a problem hiding this comment.
I noticed that too, I'll push something that should address that.
| country: Yup.string() | ||
| .required('You must select a country'), | ||
| streetAddress: Yup.string() | ||
| .max(200, 'This field cannot be longer than 200 characters') |
There was a problem hiding this comment.
Can this use the maxLength property instead of hard-coded 200?
There was a problem hiding this comment.
I don't think the maxLength property on the input itself is accessible in this scope.
What I've also noticed is that setting maxLength on the input field prevents the user from entering any more characters than that limit. So I don't think this error will even show up at all. I think the input on its own does a good job of preventing the max length from being exceeded.
Here is what I have so far for converting the AddressForm component into React. Right now this is being inserted into the CreditCardForm Angular component, but it shouldn't be too difficult to replace the Angular AddressForm throughout the app.
As a follow-up to this work, I would like to also: