Create Context Provider for Notices w/ Notices API #1843

mikejolley · 2020-03-02T14:31:13Z

This PR implements a system for creating and outputting notices for a given context.

Usage

Wrap your components in a StoreNoticesProvider component and give it a custom context.

import StoreNoticesProvider from '@woocommerce/base-context/store-notices-context';

<StoreNoticesProvider context="wc/cart">
		{children}
</StoreNoticesProvider>

Too create notices, use the useStoreNotices hook. Example:

const {
		addSuccessNotice
} = useStoreNotices();

addSuccessNotice( "Well done!" );

This hook providers a series of methods for notice manipulation under the current context:

addDefaultNotice
addErrorNotice
addWarningNotice
addInfoNotice
addSuccessNotice
removeNotices

And finally, notices are rendered in a container using the Notice component (https://github.com/WordPress/gutenberg/tree/master/packages/components/src/notice) -- these are given woocommerce class names for styling.

Screenshots

How to test the changes in this Pull Request:

Go to the cart.
Apply a coupon - see message.
Apply it again. See error.
Remove the coupon. See info message.

nerrad

Super nice job here Mike! I love how this turned out. I have a few comments needing addressed, but on the whole this looks really good.

assets/js/base/components/store-notices-container/index.js

assets/js/base/context/store-notices-context.js

nerrad · 2020-03-02T17:14:09Z

assets/js/base/hooks/use-store-cart-coupons.js

+				applyCoupon( couponCode )
+					.then( ( result ) => {
+						if ( result === true ) {
+							addSuccessNotice(
+								sprintf(
+									// translators: %s coupon code.
+									__(
+										'Coupon code "%s" has been applied to your cart',
+										'woo-gutenberg-products-block'
+									),
+									couponCode
+								),
+								{
+									id: 'coupon-form',
+								}
+							);
+						}
+					} )
+					.catch( ( error ) => {
+						addErrorNotice( error.message, {
+							id: 'coupon-form',
+						} );
+					} );


I'm fine with leaving this for now (and great job 👏 ), but it relies on actionCreators always returning a promise that resolves to the action result. There was some discussion around this behaviour in GB core at one point but it looks like that got resolved by a pull (I did!) done here. For some reason, I thought that we were eventually going to revert that but looks like it's okay. Worth noting that the discussion here, might have some impact.

I'll defer to your advice here @nerrad. The alternative was having these dispatch calls in the applyCoupon action itself (resolver.js) but it felt very opinionated to throw real notices there rather than via this hook. Any other/better suggestions I'm open.

Let's leave it, even considering the unresolved discussion I don't think it will affect this implementation.

nerrad · 2020-03-02T17:18:11Z

assets/js/base/hooks/use-store-notices.js

+	const noticesApi = useMemo(
+		() => ( {
+			addDefaultNotice: ( text, noticeProps = {} ) =>


mikejolley · 2020-03-02T17:25:24Z

@nerrad When I was working on this I noticed DEFAULT_EMPTY_ARRAY was being imported from the wrong file, but the test was using undefined instead of [] (DEFAULT_EMPTY_ARRAY). I updated that and the test here. Should pass now.

Co-Authored-By: Darren Ethier <[email protected]>

…/woocommerce-gutenberg-products-block into add/notices-provider

Co-Authored-By: Darren Ethier <[email protected]>

nerrad

Still a couple more comments but this can be merged after those are resolved so pre-approving 👍

nerrad · 2020-03-02T18:43:39Z

assets/js/base/components/store-notices-container/index.js

@@ -6,6 +6,7 @@ import classnames from 'classnames';
 import withComponentId from '@woocommerce/base-hocs/with-component-id';


This is an unused import that can be removed.

nerrad · 2020-03-02T18:45:46Z

assets/js/base/components/store-notices-container/index.js

-					</Notice>
-				)
-			) }
+		<div className={ wrapperClass } key={ instanceId }>


You don't need to have a key on the container (I should have caught this earlier). Keys are only needed on children within a specific component. My first review I was thinking the instanceId was being used for an actual ID, but it looks like it's not needed at all here?

mikejolley added 8 commits February 28, 2020 13:48

Working on store provider

7286c62

Working on store provider

c606a83

Reducer implementation

941502d

Implement core/notices

c02ba24

Add notices to store coupon hook with context

7a793b0

Improve store notice text and styling

5bc993a

Improve JS side API for notices

56d2451

Wrap functions with context additon

4957004

mikejolley added status: needs review focus: components Work that introduces new or updates existing components. skip-changelog PRs that you don't want to appear in the changelog. labels Mar 2, 2020

mikejolley requested a review from a team as a code owner March 2, 2020 14:31

mikejolley self-assigned this Mar 2, 2020

mikejolley requested review from Aljullu and removed request for a team March 2, 2020 14:31

nerrad reviewed Mar 2, 2020

View reviewed changes

Update test to []

d328b5f

mikejolley and others added 4 commits March 2, 2020 17:37

Implement props feedback and useInstanceId

f4e5467

Update assets/js/base/context/store-notices-context.js

c04688d

Co-Authored-By: Darren Ethier <[email protected]>

Merge branch 'add/notices-provider' of https://github.com/woocommerce…

3babbba

…/woocommerce-gutenberg-products-block into add/notices-provider

Update assets/js/base/context/store-notices-context.js

a148e72

Co-Authored-By: Darren Ethier <[email protected]>

nerrad approved these changes Mar 2, 2020

View reviewed changes

woocommercebot added status: ready to merge and removed status: needs review labels Mar 2, 2020

remove instance id

7564940

mikejolley merged commit da79260 into master Mar 3, 2020

mikejolley deleted the add/notices-provider branch March 3, 2020 10:26

nerrad mentioned this pull request Mar 5, 2020

Cart Checkout Flow Architectural Update (reference pull). Includes Apple Pay Integration POC #1780

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Context Provider for Notices w/ Notices API #1843

Create Context Provider for Notices w/ Notices API #1843

mikejolley commented Mar 2, 2020

nerrad left a comment

nerrad Mar 2, 2020

mikejolley Mar 2, 2020

nerrad Mar 2, 2020

nerrad Mar 2, 2020

mikejolley commented Mar 2, 2020

nerrad left a comment

nerrad Mar 2, 2020

nerrad Mar 2, 2020

		@@ -6,6 +6,7 @@ import classnames from 'classnames';
		import withComponentId from '@woocommerce/base-hocs/with-component-id';

Create Context Provider for Notices w/ Notices API #1843

Create Context Provider for Notices w/ Notices API #1843

Conversation

mikejolley commented Mar 2, 2020

Screenshots

How to test the changes in this Pull Request:

nerrad left a comment

Choose a reason for hiding this comment

nerrad Mar 2, 2020

Choose a reason for hiding this comment

mikejolley Mar 2, 2020

Choose a reason for hiding this comment

nerrad Mar 2, 2020

Choose a reason for hiding this comment

nerrad Mar 2, 2020

Choose a reason for hiding this comment

mikejolley commented Mar 2, 2020

nerrad left a comment

Choose a reason for hiding this comment

nerrad Mar 2, 2020

Choose a reason for hiding this comment

nerrad Mar 2, 2020

Choose a reason for hiding this comment