This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Capture notices from hidden block into siblings block #8390
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
senadir
added
type: bug
woocommercebot
requested review from
a team and
alexflorisca
and removed request for
a team
senadir
commented
Feb 6, 2023
Comment on lines
96
to
100
| capturedContexts={ | ||
| useBillingAsShipping | ||
| ? [ noticeContexts.SHIPPING_ADDRESS ] | ||
| : [] | ||
| } |
There was a problem hiding this comment.
I don't feel strongly about approach, another option would be to call registerContainer from this block but I didn't test that yet.
senadir
commented
Feb 6, 2023
Comment on lines
60
to
63
| const contexts = useMemo( | ||
| () => [ context, ...capturedContexts ], | ||
| [ capturedContexts, context ] | ||
| ); |
There was a problem hiding this comment.
This caused an infinite loop so that's why I added it.
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/base/components/cart-checkout/shipping-calculator/index.tsx
packages/checkout/components/store-notices-container/test/index.tsx |
|
Size Change: +162 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
alexflorisca
reviewed
Feb 7, 2023
| @@ -103,6 +103,11 @@ const Block = ( { | |||
| <AddressFormWrapperComponent> | |||
| <StoreNoticesContainer | |||
| context={ noticeContexts.SHIPPING_ADDRESS } | |||
| capturedContexts={ | |||
| useShippingAsBilling | |||
| ? [ noticeContexts.BILLING_ADDRESS ] | |||
There was a problem hiding this comment.
Do we need to take into account the WC settings where a merchant can default to the shipping address or force shipping address to the customer billing address? This seems to work ok when I tested, but I'm not sure why. The logic you have here is that the error should be displayed in the billing address context, if useShippingAsBilling is ticked. If shipping address is default, the context should be SHIPPING_ADDRESS. The notice is displayed correctly though, so I'm wondering why this works?
There was a problem hiding this comment.
useShippingAsBilling takes that into account.
The logic here is that if we have the billing form hidden and its value being synced to shipping, then we also want to capture those notices and present them in the shipping step.
The logic you have here is that the error should be displayed in the billing address context, if useShippingAsBilling is ticked.
The logic I have here is that if useShippingAsBilling is true, we want to capture the billing address errors and show them in that block (shipping address)
There was a problem hiding this comment.
Gotcha that makes more sense
| @@ -17,11 +17,11 @@ import { getClassNameFromStatus } from './utils'; | |||
| import type { StoreNotice } from './types'; | |||
|
|
|||
| const StoreNotices = ( { | |||
| context, | |||
| contexts, | |||
There was a problem hiding this comment.
You'll need to update the docs here as well to reflect plural
|
Just noticed some TS errors
|
mikejolley
reviewed
Feb 7, 2023
| @@ -91,7 +91,14 @@ const Block = ( { | |||
|
|
|||
| return ( | |||
| <AddressFormWrapperComponent> | |||
| <StoreNoticesContainer context={ noticeContexts.BILLING_ADDRESS } /> | |||
| <StoreNoticesContainer | |||
| context={ noticeContexts.BILLING_ADDRESS } | |||
There was a problem hiding this comment.
I would prefer if we could somehow define an array of context here rather than the separate prop.
mikejolley
reviewed
Feb 7, 2023
| ) => | ||
| noticesArray.findIndex( | ||
| ( _notice: Notice ) => _notice.content === notice.content | ||
| ) === noticeIndex; |
senadir
force-pushed
the
try/shipping-captures-billing-notices
branch
from
021c521 to
a99279d
Compare
alexflorisca
approved these changes
Feb 9, 2023
alexflorisca
pushed a commit
that referenced
this pull request
Feb 10, 2023
* Capture notices from hidden block into siblings block * switch to using a single context * make change bwc * add tests * support context as array in StoreNotice * move filter logic to Notice component
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recently in #8182 we introduced more controlled notice handling, this included showing notices in the correct places when needed and surfacing notices up to the main block when it makes sense.
However, this introduced a problem in sibling blocks (Shipping address billing address, payment and express payment) in which an error might be returned for both, but is rendered on one of them only while the other bubbles).
This PR adds a
capturedContextsparam to the store notice provider to capture other notices.This PR also adds filtering so duplicate errors are shown once, based on the error content only.
Fixes #8387
Testing
Changelog