Acccount detail page hook integration #995
Conversation
| const customerId = useCustomerId() | ||
| const {isRegistered} = useCustomerType() | ||
| const {data: customer} = useCustomer({customerId}, {enabled: !!customerId && isRegistered}) |
There was a problem hiding this comment.
In general I would have expected that there is a code reduction when using the hooks library or at a minimum no change line count as that was part of our goal, to move some complexities out of the project and into the commerce-sdk-react.
From the looks of it things have gotten a little more complex, one thing that is sticking out at me is that it's not easy to the customer. You have to get the id, then get the customer object, and we are also twisting in the use of this useCustomerType hook.
The funny thing is that I'm sure all the customer data is somewhere already. Have you thought of creating a utility hook in the commerce-sdk-react?
We previously talked about using some kind of app state. I think this would be a good reason to do that now.
Off of the top of me head you might have a usage like this:
const {customer, basket, others} = useAppState()
Thoughts?
There was a problem hiding this comment.
@bendvc To avoid scope creep in this PR, I've created an appStateProvider and hook to handle customer info in there for now. This will help us to achieve DRY for the customer. I'll create a ticket to move basket info in there, and we will discuss whether we want to move this provider inside commerce-react-sdk
There was a problem hiding this comment.
@kevinxh and I had a chat about useAppState for many data and have some concerns about performance issue if we aim to put many data together. Let's keep the scope to a single Provider CustomerProvider, and we are likely to have a discussion around how we want to manage state in another ticket
packages/template-retail-react-app/app/pages/account/profile.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/pages/account/index.test.js
Outdated
Show resolved
Hide resolved
…-page-details-hook-integration
| * @type {React.Context<unknown>} | ||
| */ | ||
| export const AppStateContext = React.createContext() | ||
| export const AppStateProvider = ({children}) => { |
There was a problem hiding this comment.
would it make more sense if it is called CustomerProvider? I think AppState is a bit misleading
| <CustomerProvider> | ||
| <MultiSiteProvider |
There was a problem hiding this comment.
is there a particular reason why CustomerProvider is the parent of the MultiSiteProvider? I'd imagine customer data is associate with a site on the backend, so i feel like it should be the other way around, MultiSiteProvider being the parent.
There was a problem hiding this comment.
Good catch, it should be inside MultiSiteProvider
Description
Account details page hook integration
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization