-
Notifications
You must be signed in to change notification settings - Fork 683
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
[PWA-245] Shipping Information (Authenticated) #2380
Conversation
…dio into tommy/auth-shipping-info
- Wire up mutation for address book selection
|
$cartId: String! | ||
$email: String! | ||
$address: CartAddressInput! | ||
) { | ||
setGuestEmailOnCart(input: { cart_id: $cartId, email: $email }) | ||
@connection(key: setGuestEmailOnCart) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did this work :o
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
undefined
is a perfectly valid key I guess 🤷♂️
@tjwiebell Had a look, great work overall! If we could make the following changes, it would be great!
Question - if a new default address is assigned, should that become the 1st card on the addresses page? I'm inclined to think it should. |
Is that really true for I also find it odd that we display the email field for a user that is auth'd as it has no bearing on the cart and doesn't provide any info to them aside from where they might expect to get a receipt. |
My understanding is that in Magento the name associated with the first default address for an auth'ed user is always the name on the Account, same for email. This is the use case where they have an account but do not yet have a default shipping address entered and this is the 1st time they are going through checkout after having created an account.
We are repurposing the shipping address form from guest checkout, and are using it here to simply reiterate where the order confirmation is going to go by keeping it visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work @tjwiebell
Here is a suggestion about the Form
component. Let me know if that works.
Your current implementation of the Shipping Information component is similar to this:
<ShippingInformation>
<EditForm>
<useEditForm />
<Form>
<Field />
<Field />
<Cancel />
<Submit />
</Form>
</EditForm>
</ShippingInformation>
The EditForm
component and the useEditForm
talon contain all the logic to handle 4 similar but different situations:
- Guest User Address (Add/Edit)
- Auth User Address (Add/Edit)
Each of those has differences in how the to submit is handled and what is shown for the Submit / Cancel buttons.
Due to this the talon and the component may feel bloated for a new developer when looked at.
So I was thinking about separating concerns into their own components and talons and having the EditForm
handle only the presentation of the form in each of those situations. Precisely this is what I am proposing:
<EditForm>
<Form>
<Field />
<Field />
</Form>
</EditForm>
<ShippingInformation>
<GuestUserForm>
<useGuestEditForm />
<EditForm />
<Cancel />
<Submit />
</GuestUserForm>
<AuthUserEditForm>
<useGuestEditForm />
<EditForm />
<Cancel />
<Submit />
</AuthUserEditForm>
<AuthUserAddForm>
<useGuestEditForm />
<EditForm />
<Cancel />
<Add />
</AuthUserAddForm>
</ShippingInformation>
This is just a crude idea. Maybe you can move both the auth experiences into a single component/talon but the idea is to make the talon implementation simple and easy to change using extensions in the future.
Let me know what you think.
packages/peregrine/lib/talons/CheckoutPage/AddressBook/useAddressBook.js
Outdated
Show resolved
Hide resolved
packages/peregrine/lib/talons/CheckoutPage/ShippingInformation/useShippingInformation.js
Outdated
Show resolved
Hide resolved
packages/peregrine/lib/talons/CheckoutPage/ShippingInformation/useShippingInformation.js
Outdated
Show resolved
Hide resolved
@@ -66,6 +67,12 @@ export const useCheckoutPage = props => { | |||
|
|||
const checkoutStep = checkoutData && checkoutData.cart.checkoutStep; | |||
|
|||
const toggleActiveContent = useCallback(() => { | |||
const nextContentState = | |||
activeContent === 'checkout' ? 'addressBook' : 'checkout'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any other way to handle this? Once we have Auth Payment Information, we will have to add another ternary operator to support this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My use case was just the two states so the toggle made sense. This change makes sense in the scope of Auth Payment if that will be another "tab", but I'm not familiar with that scope. There's been very little direction period about this new tab pattern, so best we get a full design before putting too much work into it.
- Animate the address card when new address is selected
export default { | ||
mutations: {}, | ||
mutations: { | ||
setDefaultAddressMutation: SET_CUSTOMER_ADDRESS_ON_CART |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You changed the definition name but not the export - I still think could be clarified by renaming it away from default
but I'll leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the scope of shippingInformation
, this mutation is only ever used to set the default address on the cart, so I've renamed to setDefaultAddressOnCartMutation
in defaf9f.
@soumya-ashok What If first time auth user is sending it as gift to some other address? ( Just making sure these kind of flows are considered :) ) |
@dpatil-magento - Even if the auth user is sending it as a gift, the account email is the one that receives the order confirmation as far as I understand. |
- Cover talon in tests
@soumya-ashok Yes, just first and last name fields! But agree, it's a edge case for sure. |
@dpatil-magento The reason I wanted to block the name fields from being edited is that the first address would be saved as their default and it seems at odds for that to be someone else's because it assumes that the default is the one they'd ship to most frequently. My preference since it is an edge case is to leave this as-is for now. |
# Conflicts: # packages/peregrine/lib/talons/CheckoutPage/useCheckoutPage.js
…ommy/auth-shipping-info
Description
As an authenticated user, during checkout my saved profile information should be auto-populated during the checkout process. Information including:
Important:
Do not display temp data from shipping estimate fields. Compare gql response to
MOCKED_ADDRESS
to verify.Notes
Notes from a team discussion on the use cases associated with this issue - for your reference Tommy Wiebell
If the shopper does not have a default address saved, the "add default address" button would take them directly to the address form in a dialog and right back to the checkout review page once they were done adding the address
All addresses added will be saved to the address book by default and the shopper will be able to remove them from the address book in the account section if they so choose
In the case that saved addresses exist, and the shopper goes from the edit link > list of addresses and adds a new one, they will go directly back to the checkout review screen after adding a new address and bypass the list of addresses, since we think it is safe to assume that they meant to use the address they just added to checkout with
The list of addresses would be presented in a "tab" and the 'add new address' form in a "dialog"
On mobile, it needs to be apparent that the 'add new address' form is an overlay on the addresses list "tab"
Related Issue
Acceptance
Verification Stakeholders
Specification
Verification Steps
Going to outline all the different flows that will need to be tested. Can flesh out more details if it isn't clear what needs to be entered.
Address Book Flows
Screenshots / Screen Captures (if appropriate)
Checklist