PURCHASE-1656: Add phone number field to Address for pickup #3075
Conversation
artsy-peril
bot
added
Jira Synced
Version: Patch
| @@ -85,7 +86,9 @@ export class AddressForm extends React.Component< | |||
| } | |||
|
|
|||
| phoneNumberInputDescription = (): string | null => { | |||
| if (this.props.billing && this.props.showPhoneNumberInput) { | |||
| if (this.props.phoneNumberOnly) { | |||
There was a problem hiding this comment.
Thoughts on adding a separate component for this phone number input instead of trying to reuse AddressForm? There's a lot of conditional logic added, seems like it would be simpler/easier to reason about if it were separate.
There was a problem hiding this comment.
I did it that way at first, but I found myself duplicating so much that this seemed like a better option
There was a problem hiding this comment.
What was duplicated? It seems like you can keep the input fields the same but create a version of this component that is pared down to just the phone number input. Would that work?
Since this component is used across the site where we need people to add shipping/billing addresses for credit cards (auctions, user settings payments, orders), it feels odd to extend it for this purpose.
We should probably (not as part of this PR 😄) re-name this component and extract it from the Order sub-app.
There was a problem hiding this comment.
Like the handlers and stuff. It just felt like a lot of duplication when I started doing it that way
There was a problem hiding this comment.
I talked it over with Ashkan and he thought this way was fine but can do it that way if you think its better
Codecov Report
@@ Coverage Diff @@
## master #3075 +/- ##
========================================
+ Coverage 82% 82% +<.1%
========================================
Files 736 736
Lines 10794 10801 +7
Branches 2160 2165 +5
========================================
+ Hits 8853 8862 +9
+ Misses 1359 1357 -2
Partials 582 582 |
| @@ -392,6 +416,7 @@ export const ShippingFragmentContainer = createFragmentContainer( | |||
| state | |||
| requestedFulfillment { | |||
| __typename | |||
|
|
|||
There was a problem hiding this comment.
This seems like it will only fetch the phoneNumber for shipping orders-- should phoneNumber be here instead of nested under that type?
There was a problem hiding this comment.
Possibly...but I get errors when I move it
There was a problem hiding this comment.
You should be able to add:
... on CommercePickup {
phoneNumber
}
There was a problem hiding this comment.
Unknown field 'phoneNumber' on type 'CommercePickup'
zephraph
added
Version: Minor
|
@lilyfromseattle I think this one can be closed. 😄 |
PURCHASE-1656 Add phone number field to Address for pickup
https://www.figma.com/file/LieDNuIuKAfhDeEPkjan4p/Checkout-Flow?node-id=0%3A1
This field should be required.
This is a refactor of this PR as was suggested in the comments
Before:
After:
