Update and select shipping rates dynamically #1794
Conversation
There was a problem hiding this comment.
Hey @senadir
So what you're done here is correct (good work on your first API addition!). Thinking about it more though, I worry this endpoint is going to be confusing to consume. The data returned is readonly, but we want to allow something to be selected. Selected/not selected doesn't really feel like part of the schema?
This is just an idea, and I think we should chat about it further (maybe over slack?) but I wonder if it would be more appropriate to either add a new endpoint for selection, or add a new property to the /cart/ endpoint itself.. That way the shipping rate endpoint here remains purely for getting rates, not for interacting with customer session data.
e.g. /cart/ already has needs_shipping property, and we could add a non-readonly chosen_shipping properly which just stores rate IDs.
Thoughts?
|
After some discussion in slack (p1582626470029500-slack-C8X6Q7XQU), here's what we arrived at:
|
senadir
force-pushed
the
add/put-method-on-shipping
branch
from
bfaf350 to
c1f1d43
Compare
|
so I did another push doing some refactors, fixed the tests errors as well. |
senadir
force-pushed
the
add/put-method-on-shipping
branch
from
295a417 to
8f5db95
Compare
senadir
force-pushed
the
add/put-method-on-shipping
branch
from
b7273b4 to
713d510
Compare
senadir
added
the
skip-changelog
senadir
changed the title
| selected = [], | ||
| shippingRates, | ||
| shippingRates = [], |
There was a problem hiding this comment.
If shippingRates is being provided as props on this component, I think selectShippingRate should be too along with selectedShipping. It's not clear to me why this needs a setSelectedShipping and selectedShipping state maintained with the Packages component. Seems like this state can be lifted up? (note the suggestions I make later about modifying the hooks which will accomplish this).
There was a problem hiding this comment.
well this is two folds, if we're going to go with the approach you’re suggesting, we will need to lift all the way up to FullCart, I don’t want this, I want to isolate hooks to appropriate components to avoid unnecessary full rerender (only packages need to rerender, not the full cart), having all hooks and all logic there will cause performance issues along the way, I would also keep shipping rates within this component if I can, but since we have preview data that is not coming from a hook, I have to pass it to full cart.
There was a problem hiding this comment.
I can move those to the top until we find a solution for mocking data store in the Editor so that we can rid of all top level hook usage.
There was a problem hiding this comment.
well this is two folds, if we're going to go with the approach you’re suggesting, we will need to lift all the way up to FullCart,
Why does it have to go all the way to FullCart? I'm suggesting you lift state up to a hook. You can still pull in the useShippingRates hook here (or in <ShippingRatesControl /> - which is where I was thinking it would be added).
I want to isolate hooks to appropriate components to avoid unnecessary full rerender (only packages need to rerender, not the full cart), having all hooks and all logic there will cause performance issues along the way
I'm wary of assertive statements like "will cause performance issues along the way", unless it's very clear an approach will cause that. All hooks and logic don't need to get implemented in the FullCart and then all props get pushed down.
but since we have preview data that is not coming from a hook, I have to pass it to full cart.
I'm not sure I understand the relevance here? Why not just pass along an isEditor prop from the <Editor/> component to full cart and that can be used to determine whether shipping rates come from the preview data or not?
There was a problem hiding this comment.
Personally I'd avoid having preview handling in on this pull as it could block it from getting merged unnecessarily.
There was a problem hiding this comment.
assuming we’re getting shipping rates usinguseShippingRates, we will have the mixed preview data thing, I think we can the useSelectShippingRates call in packages (the nearest place they’re used in), and refactor in the future?
There was a problem hiding this comment.
I'm wary of assertive statements like "will cause performance issues along the way", unless it's very clear an approach will cause that. All hooks and logic don't need to get implemented in the FullCart and then all props get pushed down.
While there is no direct implications that are obvious, having a lot of logic in the root is going to be problematic for slower devices, as a rule of thumb, we should always try to avoid unnecessary rerenders (our Cart block rerenders 3 or 4 times on initial load only)
There was a problem hiding this comment.
we should always try to avoid unnecessary rerenders
I agree, but premature optimization can be expensive too. Re-renders aren't necessarily bad, it's expensive re-renders that are bad. So let's avoid premature optimization unless it's clear expensive re-rendering is happening.
| export const useSelectShippingRate = () => { | ||
| const { selectShippingRate } = useDispatch( storeKey ); | ||
| return selectShippingRate; | ||
| }; |
There was a problem hiding this comment.
I think this should receive shippingRates as an argument and then expose selectedShippingRates and selectShippingRate. Then you can include this in the useShippingRates hook. That way this particular hook contains the logic for extracting the selected rates from shippingRates. So something like:
export const useSelectShippingRate = ( shippingRates ) => {
const initialSelected = shippingRates.map(
( p ) => p.shipping_rates.find( ( rate ) => rate.selected )?.rate_id
);
const [ selectedShippingRates, setSelectedShipping ] = useState(
initialSelected
);
const { selectShippingRate } = useDispatch( storeKey );
const setRate = ( newShippingRate, packageId ) => {
setSelectedShipping( [ newShippingRate ] );
selectShippingRate( newShippingRate, packageId );
};
return {
selectShippingRate: setRate,
selectedShippingRates,
};
};Then in useShippingRates:
export const useShippingRates = () => {
const { shippingRates } = useStoreCart();
const { selectShippingRate, selectedShippingRates } = useSelectShippingRate( shippingRates );
/** rest of existing logic and you return the extra interfaces **/
}There was a problem hiding this comment.
Incidentally, this is where you could put preview data if you wanted in the editor context. useShippingRate could optionally receive shippingRates as an argument and if it's present then skip using what's returned from the cart (although it'd probably be better to just have any preview data just populate the store and turn off resolving via an isEditor flag maybe. Lot's of ways to do it).
There was a problem hiding this comment.
although it'd probably be better to just have any preview data just populate the store and turn off resolving via an isEditor flag maybe.
I love this, but we still including preview data to the frontend, but I could argue that it has very minimal weight consideration, the idea was two have possibly two data stores (one registered in the editor and the other frontend, while one resolves to actual data, the other resolves to preview data, but this is a considerate change)
There was a problem hiding this comment.
Let's not have preview data be dealt with in this pull. How it's implemented may depend to a degree on what gets implemented from the work I've been doing as well.
| // Update customer session. | ||
| WC()->customer->set_props( | ||
| array( | ||
| 'shipping_country' => isset( $request['country'] ) ? $request['country'] : null, |
There was a problem hiding this comment.
Is this check needed for $request['country']? It should be guaranteed to be set or it would have thrown an error when validated right?
There was a problem hiding this comment.
keeping the same style as the rest? I agree that it’s not needed.
senadir
force-pushed
the
add/put-method-on-shipping
branch
2 times, most recently
from
eff7d7e to
ac02b7e
Compare
There was a problem hiding this comment.
Good job with this @senadir! This was a complex task touching many different areas of the codebase. I did some testing and found some small issues.
When opening a page with the Cart and Checkout blocks, I see a warning in the devtools:
When writing into the State field of the Checkout block, I get some JS errors:
When loading the Checkout block without any address introduced yet, Shipping options are permanently loading:
* Items should not have keys in API response * Include package ID in response (this is just a basic index) * /cart/select-shipping-rate/package_id * Add package_id to package array * Update responses and add shipping-rates to main cart endpoint * update-shipping endpoint
* add selecting shipping to store * directly call useSelectShippingRate * refactor cart keys transformation to reducer * remove selecting first result and accept selecting * move update shipping to new endpoint * pass selected rates down * select shipping right directly and fix editor issues
senadir
force-pushed
the
add/put-method-on-shipping
branch
from
feb9b78 to
4ba8067
Compare
REST API
Some changes were made to the
store/cart/endpoint, any endpoint in cart will also return the whole newly updated cart (exceptcart/shipping-rates, which is still independent), alongside that, some endpoints have been moved around, and changed, here is the summary:/cart/shipping-ratesworks as usual, but it’s not currently used in the cart, it takes an address, it returns rates only./cart/returns an array of shipping-rates for convenience.shipping-ratesresource have aselectedproperty much now./cart/update-shippingtakes an address, updates customer session, and returns the cart with new rates./cart/select-shipping-rate/<package_id>lets you set a rate id for a package by POSTing, it also returns the full cart.(majority of changes were done in #1833).
GET /wc/store/cartresponse
POST /wc/store/cart/update-shippingresponse
GET /wc/store/cart/shipping-ratesresponse
GET /wc/store/cart/select-shipping-rate/0response
wc/store/cartdata store.(a majority of changes were done in #1829).
actions
Some new actions have been added:
selectShippingRateselects a shipping rate from a package, updates the whole store.updateShippingupdates the shipping address, update the whole as well.receiveShippingAddressused to track the act of updating the address.reducer
UPDATING_SHIPPINGto track the shipping update stateselectors
areShippingRatesLoadingused to track the state of loading and updating rates.Our ever-growing collection of hooks.
useSelectShippingRatereturns theselectShippingRateaction dispatcher, this used to have more things in it, but I think we can skip it and directly haveuseDispatchinside our component cc @nerraduseShippingRatesdoesn’t use collections anymore, it has its own resolvers, it returns the same as beforeshippingRatesshippingRatesLoadingand alsoupdateShippingan action dispatcher to update shipping address.useStoreCartalso returnsshippingRatesnow,useShippingRatesrelies on it.Misc
destinationnow.How to test this:
Set up a page with Cart, some product in it the cart, and some shipping rates for different zones.
1- Go the cart, see if there are any rates or not.
2- Update your shipping address to one that has rates.
3- Rates should show up and one of them should be initially selected.
4- Select a different rate, it will select immediately, and prices will changes.
5- Reload the page, your selection should persist, your address should persist as well.
fixes #1531 #1836 #1779