-
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-240] [feature]: Cart Price Summary #2092
Conversation
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
|
3e71954
to
f05a8c3
Compare
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
5adb2cc
to
d6a0984
Compare
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
d6a0984
to
1021135
Compare
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
) : null; | ||
}; | ||
|
||
// TODO: Gift cards are only enabled in EE, write a build time tool that turns the component into a no-op and the static fragments into __typename requests. |
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.
This is a big TODO. Before we merge this PR we have to have a way to determine which fragment to use, otherwise we risk breaking CE.
DiscountSummary.fragments = { | ||
discounts: gql` | ||
fragment _ on CartPrices { | ||
discounts { |
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 2.3.3 we have discount
, which is a "deprecated but functional" field in 2.3.4
In 2.3.4 we have discounts
, which 2.3.3 does not have.
If we want to support both versions we will need to provide some way (build or runtime?) to determine which field is valid for the current backend.
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 going to proceed with the assumption that we don't need to support 2.3.3, so discounts
is valid.
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 going to proceed with the assumption that we don't need to support 2.3.3, so
discounts
is valid.
We do need to support 2.3.3
.
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
variables: { | ||
cartId | ||
}, | ||
fetchPolicy: 'no-cache' |
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.
We should verify that actions that modify the price summary values such as shipping method or adding a shipping address elsewhere also update the price summary even if no-cache
is used here.
packages/venia-ui/lib/components/CartPage/PriceSummary/priceSummary.js
Outdated
Show resolved
Hide resolved
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
…tudio into rugh/price-summary
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
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 to me. I'd like to know what effect fraql
has on the bundle before approving this; I'm fine with it from a fragment authoring perspective.
Also note that we do have to support 2.3.3.
DiscountSummary.fragments = { | ||
discounts: gql` | ||
fragment _ on CartPrices { | ||
discounts { |
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 going to proceed with the assumption that we don't need to support 2.3.3, so
discounts
is valid.
We do need to support 2.3.3
.
I have removed it and used named exports instead. This just leaves a little coupling magic in terms of fragment name and the name of the export but that's not a big deal, and is preferable over using a new dependency IMO.
Rather than go back and forth on this, let's put that in documentation somewhere and make sure the entire team knows exactly what backend version we have to support when we are coding. I spoke with @tjwiebell and we both thought that this code wouldn't be released until our next release which would coincide with 2.3.5. And we both thought that we were supporting "current - 1", so 6.0.0 would have to support 2.3.5 and 2.3.4 whereas 5.0.0 which is releasing now will have to support 2.3.4 and 2.3.3 In regards to this specific PR, I can work around that by using the |
Yes, my mistake. This can be for 2.3.4, so we're good here. 👍 |
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
QA Pass, can be merged once re-approved. |
Description
Adds a price summary to the cart page. Each of the more "complex" line items have been broken into separate components. To avoid having "magical" named queries being used in the parent I added a dependency called fraql that allows you to create unnamed queries. Look at
priceSummary.js
for usage example.Related Issue
Closes PWA-240.
Acceptance
Verification Stakeholders
Specification
Verification Steps
For reference, use the tutorial for checkout w/ graphQL.
Empty cart
Go to the
/cart
page. The summary should not appear when there are no items.With items
/cart
page. You should see an updated summary.Scrolling view
The summary should scroll with the contents of the cart page.
Note
Coupons values are represented in an array with other possible values and no way to identify it as such. For now, I have represented this as a "Discount" line item.
Gift Cards can be enabled/disabled at build time if we're on EE or not, but that work needs to be done. In this PR they are disabled entirely with a local flag but we should address that soon. Deferred the work to PWA-78 for now.
Checklist