-
Notifications
You must be signed in to change notification settings - Fork 673
Conversation
Deploy preview for saleor-storefront-stage processing. Building with commit 62d083a https://app.netlify.com/sites/saleor-storefront-stage/deploys/5eafd4d6bc5ff0000906fb12 |
src/@sdk/react/components/CheckoutProvider/CheckoutProvider.tsx
Outdated
Show resolved
Hide resolved
src/@sdk/react/components/CheckoutProvider/CheckoutProvider.tsx
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/@next/components/molecules/CheckoutProgressBar/CheckoutProgressBar.tsx
Show resolved
Hide resolved
src/@next/components/organisms/AddressGridSelector/AddressGridSelector.tsx
Outdated
Show resolved
Hide resolved
src/@next/components/organisms/BraintreePaymentGateway/styles.ts
Outdated
Show resolved
Hide resolved
export const Wrapper = styled.div``; | ||
|
||
export const Section = styled.section``; | ||
|
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.
...why?
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'll fix it, but in some cases I see that it may facilitate creating new components, where you might not know in advance which html tags should be styled and which should not, so the assumption that we do all styled tags eliminates the need to change tags in JSX and move them to styles.ts
file on a regular basis. Eventually every styled tag is rendered as single html tag, so it does not change a lot.
return ( | ||
<S.Section> | ||
<S.Title data-cy="checkoutPageSubtitle">SHIPPING METHOD</S.Title> | ||
<Formik |
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 believe that Formik has useFormik
hook now, we should probably use it
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.
Yes, it might be much better, but I guess we may switch to useFormik
in another PR as many components are already prepared with render props way
export interface IShippingMethodPrice { | ||
/** | ||
* Currency code. | ||
*/ | ||
currency: string; | ||
/** | ||
* Amount of money. | ||
*/ | ||
amount: number; | ||
} |
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.
It looks like it could be replaced by some gql fragment type. Actually, a lot of props in this PR can be written using some fragments or types coming from queries.
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'm not sure about it, because if we want to rely on SDK in a separate package in the future, it might hide from us those interfaces generated by Apollo in that package.
src/@next/components/organisms/DummyPaymentGateway/DummyPaymentGateway.tsx
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This pull request fixes 5 alerts when merging 1cf7c8a into 3cfdeef - view on LGTM.com fixed alerts:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This pull request fixes 5 alerts when merging 62d083a into 3cfdeef - view on LGTM.com fixed alerts:
|
I want to merge this change because... it adds stateful checkout and cart to SDK with new cart and checkout UI.
New checkout and cart UI
New checkout and cart UI with completely refactored components which uses new hooks within them.
New hooks for checkout and cart
useCheckout
anduseCart
hooks, which has all data and methods associated with checkout and cart to use within React components:SDK changes
SaleorCheckoutAPI
andSaleorCartAPI
classes aggregated inSaleorAPI
class. State for those API classes is derived from newSaleorState
object, which is altered based on results form GraphQL queries/mutations, local storage changes and job executions. All data fromSaleorState
is observable, what means thatSaleorCheckoutAPI
andSaleorCartAPI
classes observes appropriate data from there and automatically recalculate its own state based on notified changes. After that,SaleorAPI
is notified about changes, which in turn notifies react context provider about them. Context provider then recreates context object with references toSaleorCheckoutAPI
andSaleorCartAPI
classes, which are used by new hooks. Effectively, React triggers components tree update and all instances of new hooks are up to date.SaleorManager
, i.e.:SaleorProvider
which providesSaleorAPI
in context, which is used among all new React hooks, i.e.:Error handling
There are two types of functions returned by
useCart
anduseCheckout
hooks:completeCheckout()
), which return promise with:addItem()
):For queued functions, errors might be catched, by adding error listeners:
Fixes