-
Notifications
You must be signed in to change notification settings - Fork 219
Conversation
@@ -82,6 +82,7 @@ | |||
top: 0; | |||
text-align: center; | |||
transform: translateX(-50%); | |||
white-space: nowrap; |
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.
The release ZIP for this PR is accessible via:
Script Dependencies ReportThere is no changed script dependency between this branch and trunk. This comment was automatically generated by the TypeScript Errors Report
assets/js/base/components/cart-checkout/shipping-rates-control/index.tsx
assets/js/base/context/hooks/payment-methods/use-payment-method-interface.ts assets/js/base/context/hooks/shipping/use-shipping-data.ts assets/js/base/context/providers/cart-checkout/shipping/index.tsx assets/js/base/context/providers/cart-checkout/shipping/reducers.js assets/js/blocks/checkout/inner-blocks/checkout-pickup-options-block/block.tsx packages/checkout/components/store-notices-container/test/index.tsx |
} | ||
onSelectRate={ ( newShippingRateId ) => { | ||
selectShippingRate( newShippingRateId, packageId ); | ||
dispatchCheckoutEvent( 'set-selected-shipping-rate', { |
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.
Moved here from useSelectShippingRate
which I've merged with useShippingData
. The 2 hooks did very similar things.
Size Change: +519 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
@@ -21,10 +21,10 @@ export const hasStoreNoticesContainer = ( container: string ): boolean => { | |||
}; | |||
|
|||
const findParentContainer = ( container: string ): string => { | |||
if ( container.includes( noticeContexts.CHECKOUT + '/' ) ) { | |||
if ( container.includes( noticeContexts.CHECKOUT ) ) { |
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.
Removed the slash do that wc/cart
errors surface in wc/checkout
if the StoreNoticesContainer
is not present.
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.
Could you explain this a bit more, not sure I understand fully what the slash did and why removing it is needed
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.
The logic was looking for a prefix of wc/cart/
and then selecting either the parent container, or switching to wc/checkout
.
This worked for children of wc/cart/
but not for wc/cart
itself. Removing the slash lets it change the container if the wc/cart
container was passed in.
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.
Ahhh right yes because we add errors to inner blocks. Nice one thanks!
} | ||
|
||
// Re-throw the error. | ||
throw error; |
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.
These did nothing. We cannot catch them because the function is async
.
@@ -47,7 +47,8 @@ const StoreNotices = ( { | |||
|
|||
if ( | |||
activeElement && | |||
inputs.indexOf( activeElement.tagName.toLowerCase() ) !== -1 | |||
inputs.indexOf( activeElement.tagName.toLowerCase() ) !== -1 && | |||
activeElement.getAttribute( 'type' ) !== 'radio' |
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.
Small change allows us to scroll to errors after clicking a radio input.
@@ -0,0 +1,42 @@ | |||
/** |
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.
These types were all moved from the old type-defs
@@ -269,14 +256,6 @@ export const removeCoupon = | |||
dispatch.receiveCart( response ); | |||
} catch ( error ) { | |||
dispatch.receiveError( error ); | |||
|
|||
// If updated cart state was returned, also update that. | |||
if ( error.data?.cart ) { |
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.
Moved this logic into receiveError
thunk.
@@ -7,7 +7,7 @@ import type { CartResponse } from './cart-response'; | |||
export type ApiErrorResponse = { | |||
code: string; | |||
message: string; | |||
data: ApiErrorResponseData; | |||
data?: ApiErrorResponseData | undefined; |
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.
Not all API errors have data - it's optional.
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 looks good, when testing with die( 1 )
it shows
Could we make this more user-friendly, like "Something went wrong, please contact us for assistance." instead?
I also noticed that when retrying on the same page, after causing the generic error, and then trying with the thrown named error, the generic error still remains. Is it possible to get around this? I think this is the same problem as we had with the quantity messages, vs auto-dismissed and persistent errors isn't it?
@@ -21,10 +21,10 @@ export const hasStoreNoticesContainer = ( container: string ): boolean => { | |||
}; | |||
|
|||
const findParentContainer = ( container: string ): string => { | |||
if ( container.includes( noticeContexts.CHECKOUT + '/' ) ) { | |||
if ( container.includes( noticeContexts.CHECKOUT ) ) { |
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.
Could you explain this a bit more, not sure I understand fully what the slash did and why removing it is needed
Similar issue @opr Will the PR you're working on which dismisses notices work here too? |
887b3fe
to
be006fa
Compare
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.
Functionality-wise this is working ok according to test instructions, but I added a few comments please take a look
assets/js/base/context/providers/cart-checkout/shipping/index.tsx
Outdated
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.
Looks good now, the test comment could maybe be updated to reflect that it's the number of elements containing matching text, rather than instances of the text occurring?
Besides that, nice one. Thanks for working on this!
Overlapping functionality and responsibility easily merged into a single hook.
… original context
Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com>
Co-authored-by: Thomas Roberts <5656702+opr@users.noreply.github.com>
6daf0cf
to
1154fc2
Compare
e2e fails seem to be fse related. JS tests are passing here and for me locally, so merging. |
We identified an issue that API responses that returned errors were being stored to the data stores, but not converted to notices.
To resolve this, I've converted
receiveError
to a thunk that creates error notices, following the pattern ofreceiveCart
/notifyQuantityChanges
that @opr implemented.When detangling this problem I converted a few files to Typescript and merged some hooks. Ill add notes in the diff.
Fixes #7721
Other Checks
Testing
Automated Tests
User Facing Testing
We need to test this by throwing errors from the API. Add some error code here:
woocommerce-blocks/src/StoreApi/Routes/V1/CartSelectShippingRate.php
Line 74 in 95e4604
You can try:
die( 1 );
to test a generic errorthrow new RouteException( 'test-error', 'Test', 400 );
to test a named errorThere is no other error handling or retry mechanism beyond this (out of scope).
Since this revises the changes in #7363, repeat these testing steps also:
WooCommerce Visibility