-
Notifications
You must be signed in to change notification settings - Fork 679
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
[chore] Checkout Page gql updates #2254
Conversation
Signed-off-by: sirugh <rugh@adobe.com>
…-working query, mutation, and resolvers for adding checkoutStep to Apollo client state. Signed-off-by: sirugh <rugh@adobe.com>
|
…t step Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
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.
Looks good. I'll have to test it out locally a bit later.
packages/venia-ui/lib/components/CheckoutPage/checkoutPage.gql.js
Outdated
Show resolved
Hide resolved
packages/venia-ui/lib/components/CheckoutPage/PaymentInformation/paymentInformation.css
Show resolved
Hide resolved
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.
I love the stepper state but one of the important user flow is failing. Please check my comments.
const { data: stepData, client } = useQuery(getCheckoutStepQuery); | ||
const setCheckoutStep = useCallback( | ||
step => { | ||
client.writeData({ data: { checkoutStep: step } }); |
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.
I don't think Apollo Cache is the right place to store this information. For instance,
- Login and add something to your cart. Go to the checkout page and go to the review step (Step 3).
- Now logout.
- As a guest add something to your cart and go to the checkout page. Instead of treating you as a new customer, it will treat you as an existing customer and take you directly to the review step (Step 3) even though you haven't entered the prev step's data.
Either clear the data:
- when the user logs out
- when the
cartId
changes - when the user leaves the checkout page
or do not use Apollo's cache, use local state so every time you go to the checkout page, it has to treat you as a new customer even though the component has all the data it needs to render the Step (Assuming that is the intended process).
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.
I discussed the UX with @soumya-ashok and she said that a user, upon a refresh, should return to the step that they were last on. So we do need some sort of persistence layer that "remembers" where the user was. And that can come from the server or it can come from some local state that is simply persisted after a user proceeds through a step.
As for when to clear the data, I added an unmount effect that should have set the value, but I would also have expected the user logging out to clear the cache entirely. I'm surprised that it persisted the checkout step. I'll add the cartId
change suggestion to the effect and I'll test the user log out scenario myself to see what I need to do. Thanks!
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.
Ok, instead of taking care of this here I think we should try to solve the larger problem of our cache not resetting on logout. I opened https://jira.corp.magento.com/browse/PWA-457 for this.
That said, I don't think your step 3 is actually occurring, or at least I don't see how it did. The checkout page resets the step on unmount so when you left the page to add a new item to your cart the step should have been reset to 1.
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.
I am able to reproduce it. Let's deal with this in PWA-457.
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.
Your video suggests a bug - I'll fix it in the PR if I can.
const classes = mergeClasses(defaultClasses, props.classes); | ||
|
||
// TODO: Replace "doneEditing" with a query for existing data. | ||
const [doneEditing, setDoneEditing] = useState(false); |
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.
Shouldn't these couple lines of code go into a talon?
And I am not sure if you can replace doneEditing
for a data query because the presence of data is not the same as the completion of the step which doneEditing
signifies.
For instance, if a logged-in user lands at the checkout page and if you query for the data, you will get it back if the user has default shipping and payment information set for their account. Unless if the data you are talking about is cart scoped in which case, you are fine.
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.
Shouldn't these couple lines of code go into a talon?
Yep! These were just temporary additions. I assumed whomever took the component ticket would move the logic for this component into a talon.
And I am not sure if you can replace doneEditing for a data query because the presence of data is not the same as the completion of the step which doneEditing signifies.
This state was simply meant to signal whether to render in "edit mode" or in "read only mode". If the cart has payment information already, this component should render as if it was in read only mode. Same for the rest of the components.
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.
If the data is cart scoped then we are good.
packages/venia-ui/lib/components/CheckoutPage/ShippingInformation/shippingInformation.js
Show resolved
Hide resolved
packages/venia-ui/lib/components/CheckoutPage/ShippingMethod/shippingMethod.js
Show resolved
Hide resolved
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
…al state for receipt data for one-time receipt display after checkout Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
@sirugh - Ran into two things QA'ing:
|
The problem is that the "review order" button is a child of the payment information since it cannot be enabled until payment information is completed. The price summary is a sibling to payment information, not a child, so the positioning gets tricky. When we're in desktop view, it doesn't matter, order summary is always on the right. But when we collapse to a single column, how do I control the order of the price summary and the payment info components?
This is because the "done" state is actually up to the components. If you refresh the page or go back to the page I assume that the data has been submitted and the components will render in their correct views ie "edit" or "read only". To add to this, guest and auth checkout have different flows, which is confusing to develop against. For a guest, on your first visit to checkout you see a blank form and the shipping info form. If you fill out shipping info, it turns into a "read only" card view and you are shown the next entry form for shipping methods. For auth, on your first visit to checkout, all of the sections are cards with an "edit" button. I really wish these two scenarios were not so different for the same page. |
QA Approved ✅ Will add acceptance criteria to those component Stories. |
Description
Some minor refactoring of checkout page in preparation for the team.
Noteable changes:
useState
.Related Issue
Closes PWA-395.
Acceptance
Verification Stakeholders
Specification
Verification Steps
Screenshots / Screen Captures (if appropriate)
Checklist