Skip to content
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

Merged
merged 6 commits into from
Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,14 @@ const Block = ( {

return (
<AddressFormWrapperComponent>
<StoreNoticesContainer context={ noticeContexts.BILLING_ADDRESS } />
<StoreNoticesContainer
context={ noticeContexts.BILLING_ADDRESS }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer if we could somehow define an array of context here rather than the separate prop.

capturedContexts={
useBillingAsShipping
? [ noticeContexts.SHIPPING_ADDRESS ]
: []
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel strongly about approach, another option would be to call registerContainer from this block but I didn't test that yet.

/>
<AddressForm
id="billing"
type="billing"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ const Block = ( {
<AddressFormWrapperComponent>
<StoreNoticesContainer
context={ noticeContexts.SHIPPING_ADDRESS }
capturedContexts={
useShippingAsBilling
? [ noticeContexts.BILLING_ADDRESS ]
Copy link
Member

@alexflorisca alexflorisca Feb 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha that makes more sense

: []
}
/>
<AddressForm
id="shipping"
Expand Down
27 changes: 23 additions & 4 deletions packages/checkout/components/store-notices-container/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '@woocommerce/block-data';
import { getNoticeContexts } from '@woocommerce/base-utils';
import type { Notice } from '@wordpress/notices';
import { useMemo } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -24,9 +25,19 @@ const formatNotices = ( notices: Notice[], context: string ): StoreNotice[] => {
} ) ) as StoreNotice[];
};

const removeDuplicateNotices = (
notice: Notice,
noticeIndex: number,
noticesArray: Notice[]
) =>
noticesArray.findIndex(
( _notice: Notice ) => _notice.content === notice.content
) === noticeIndex;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 This makes sense.


const StoreNoticesContainer = ( {
className = '',
context,
capturedContexts = [],
additionalNotices = [],
}: StoreNoticesContainerProps ): JSX.Element | null => {
const { suppressNotices, registeredContainers } = useSelect(
Expand All @@ -46,23 +57,31 @@ const StoreNoticesContainer = ( {
subContext.includes( context + '/' ) &&
! registeredContainers.includes( subContext )
);

const contexts = useMemo(
() => [ context, ...capturedContexts ],
[ capturedContexts, context ]
);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This caused an infinite loop so that's why I added it.

// Get notices from the current context and any sub-contexts and append the name of the context to the notice
// objects for later reference.

const notices = useSelect< StoreNotice[] >( ( select ) => {
const { getNotices } = select( 'core/notices' );

return [
...unregisteredSubContexts.flatMap( ( subContext: string ) =>
formatNotices( getNotices( subContext ), subContext )
),
...capturedContexts.flatMap( ( capturedNotice: string ) =>
formatNotices( getNotices( capturedNotice ), capturedNotice )
),
...formatNotices(
getNotices( context ).concat( additionalNotices ),
context
),
].filter( Boolean ) as StoreNotice[];
]
.filter( removeDuplicateNotices )
.filter( Boolean ) as StoreNotice[];
} );

if ( suppressNotices || ! notices.length ) {
return null;
}
Expand All @@ -71,7 +90,7 @@ const StoreNoticesContainer = ( {
<>
<StoreNotices
className={ className }
context={ context }
contexts={ contexts }
notices={ notices.filter(
( notice ) => notice.type === 'default'
) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ import { getClassNameFromStatus } from './utils';
import type { StoreNotice } from './types';

const StoreNotices = ( {
context,
contexts,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to update the docs here as well to reflect plural

className,
notices,
}: {
context: string;
contexts: string[];
className: string;
notices: StoreNotice[];
} ): JSX.Element => {
Expand Down Expand Up @@ -67,11 +67,11 @@ const StoreNotices = ( {

// Register the container context with the parent.
useEffect( () => {
registerContainer( context );
contexts.map( ( context ) => registerContainer( context ) );
return () => {
unregisterContainer( context );
contexts.map( ( context ) => unregisterContainer( context ) );
};
}, [ context, registerContainer, unregisterContainer ] );
}, [ contexts, registerContainer, unregisterContainer ] );

// Group notices by whether or not they are dismissible. Dismissible notices can be grouped.
const dismissibleNotices = notices.filter(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export interface StoreNoticesContainerProps {
context: string;
// List of additional notices that were added inline and not stored in the `core/notices` store.
additionalNotices?: ( NoticeType & NoticeOptions )[];
capturedContexts: string[];
}

export type StoreNotice = NoticeType & NoticeOptions;