google-listings-and-ads Add useEffect to avoid multiple CES prompts on the Dashboard Page

Changes proposed in this Pull Request:

Closes #1428 .

This PR fixes the issue with the CES prompts in the dashboard page after the campaign creation success modal is shown. The issue was caused because the status isCESPromptOpen stays always true when navigating through the dashboard sub-components such as EditFreeCampaign, EditPaidAdsCampaign and CreatePaidAdsCampaign, therefore dispatching a new CustomerEffortScore notice when returns to the dashboard page.

https://github.com/woocommerce/google-listings-and-ads/blob/74949bc72a41ff79d47138c9e319739ff4ba36aa/js/src/dashboard/index.js#L35

Before this PR

https://user-images.githubusercontent.com/2488994/171261339-72fdff1a-c1da-4e02-ab9a-63ef19bd2022.mp4

After this PR

https://user-images.githubusercontent.com/2488994/171262126-d2165848-1dca-47d7-9518-6a5bb476de5e.mov

Detailed test instructions:

Go to wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&guide=campaign-creation-success
Click in "Got it"
One CES prompt should be displayed.
Visit Edit free listings, Edit Ads Campaigns and Create Ads campaigns and return to the dashboard page and it should show only one CES prompt.

Changelog entry

Fix - Multiple CES prompts in the Dashboard Page

May 31 '22 18:05 jorgemd24

Thanks @ecgan for the detailed testing!

@jorgemd24 I just tested it and found the CES prompt will become nonfunctional after going to the subpath. However it's not because of the changes in your PR, it's an existing but undiscovered problem in my PR #1152. Check the video:

https://user-images.githubusercontent.com/914406/171572446-a5158e98-301f-4fef-944f-69cfb0eca4f4.mov

I don't have a solution in my head at the moment, if you think it's fun to solve it please go ahead otherwise I'd try to solve it after my sprint tasks are done as it's a bug I created 🐛 😅.

@ecgan

I'm thinking it is because we want to retain the modal in the first tab so that users can click on the next button, but from the user experience perspective, it feels weird (and maybe even appear buggy) to open a new tab showing the same page.

I searched a bit from the previous commits and found it's a requirement in #555, quotes from @j111q:

This link opens the underlying Product Feed page for Google Listings & Ads in a new tab.

I know that’s a little weird, but we’re trying to achieve two things here - people’s desire to manage their feed AND our objective to get them to create paid campaigns in the next screen.

So if they do want to edit their product feed, they have it in another tab.

Jun 02 '22 07:06 ianlin

@ecgan,

Thanks for your suggestions 🙂 , related to:

A note for the future: I think the fix here is more like a quick fix workaround. For a long term better solution, I think we should have an AppShell that contains the whole plugin, and things like the CustomerEffortScorePrompt, DifferentCurrencyNotice and NavigationClassic components should be in the AppShell, so that there will only be one instance of them throughout the whole app. With that approach, we wouldn't need to have this useEffect call.

I agree with you, the problem is because every time that CustomerEffortScorePrompt is rendered is dispatching a new Notice, using the AppShell, the dispatch action would be triggered only once.

Package: @woocommerce/customer-effort-score

export default compose(
	withDispatch( ( dispatch ) => {
		const { createNotice } = dispatch( 'core/notices2' );

		return {
			createNotice,
		};
	} )
)( CustomerEffortScore );

I would suggest creating a separate issue where we can discuss how to implement the AppShell and which components could get the advantage of that improvement. Also, it would be convenient to see how other plugins are implementing this.

This approach would also solve the issue where the notice appears to keep re-appearing when we switch tabs, see a demo video of the issue below:

That issue is affecting others' notices, I am not sure if this is a "bug" in our app or if it is something more related to core/notices. See video (I intentionally blocked the ads/campaigns endpoint so we can get the prompt):

https://user-images.githubusercontent.com/2488994/171616024-3fb3352b-7dd3-4b10-b75e-cbdd2a3b15c1.mp4

@ianlin

I don't have a solution in my head at the moment, if you think it's fun to solve it please go ahead otherwise I'd try to solve it after my sprint tasks are done as it's a bug I created 🐛 😅.

Thanks for your comments, I think I know what is the problem:

The CustomerEffortScorePrompt component is mounted.
The CES prompt is dispatched.
'Give feedback' has two actions linked to the CustomerEffortScore component: setVisible(true) and onModalShownCallback().

Package: @woocommerce/customer-effort-score

	useEffect( () => {
		if ( ! shouldCreateNotice ) {
			return;
		}

		createNotice( 'success', label, {
			actions: [
				{
					label: __( 'Give feedback', 'woocommerce-admin' ),
					onClick: () => {
						setVisible( true );
						onModalShownCallback();
					},
				},
			],
			icon,
			explicitDismiss: true,
			onDismiss: onNoticeDismissedCallback,
		} );

		setShouldCreateNotice( false );

		onNoticeShownCallback();
	}, [ shouldCreateNotice ] );

The IU changes to a different page, for example, editing Campaign ads.
CustomerEffortScorePrompt and CustomerEffortScore are unmounted .
The prompt will still show but is missing its CustomerEffortScore component because was unmounted
Go back to the dashboard, a new CustomerEffortScorePrompt is mounted, 'Give feedback' works but the previous CES prompt does not work because does not have any CustomerEffortScore component associated with it (it was unmounted before).

This issue will affect the current solution too, I will see how we can fix this as well.

Let me know your thoughts!

Jun 02 '22 12:06 jorgemd24

@jorgemd24 ,

I would suggest creating a separate issue where we can discuss how to implement the AppShell and which components could get the advantage of that improvement.

Yup, I have the same thought too. I have created issue https://github.com/woocommerce/google-listings-and-ads/issues/1548 for this. It is an "early proposal" issue, feel free to refine the issue description and add more things into it. 😄

I think I know what is the problem: [...]

The prompt will still show but is missing its CustomerEffortScore component because was unmounted

I haven't looked detailed into the code there yet, but I have a feeling that @woocommerce/customer-effort-score has a leaky abstraction. It is nice to have a <CustomerEffortScorePrompt> React component that will display a notice when we render the component, but if we use the "React way of doing things", when <CustomerEffortScorePrompt> is unmounted / not rendered, the notice should be dismissed / not displayed, too. Otherwise, it gives us this sort of problems because we are mixing React and non-React way of doing things.

An idea to address the above is we can probably come up with a wrapper around CustomerEffortScore, so that it works like React way. When it works good enough for us, we can contribute it upstream into @woocommerce/customer-effort-score package.

Jun 02 '22 16:06 ecgan

@ecgan,

I haven't looked detailed into the code there yet, but I have a feeling that @woocommerce/customer-effort-score has a leaky abstraction. It is nice to have a <CustomerEffortScorePrompt> React component that will display a notice when we render the component, but if we use the "React way of doing things", when <CustomerEffortScorePrompt> is unmounted / not rendered, the notice should be dismissed / not displayed, too

Yes, currently (using develop/trunk) the prompt is throwing an error when you are changing the page and clicking on "Give feedback".

Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function.

See the following videos:

https://user-images.githubusercontent.com/2488994/172468682-d2ab6435-e495-4e34-89f1-78f4d8cf3538.mp4

https://user-images.githubusercontent.com/2488994/172468307-cc176a25-2823-430a-b080-00b5db8b8b78.mp4

@ianlin Does the prompt need to be shown on every page? or only in the dashboard/product feed page?

I have been playing with CustomerEffortScorecomponent from @woocommerce/customer-effort-score, and I think the easiest way to have the CustomerEffortScore working as "React", so when CustomerEffortScore is unmounted, the notice dissapered too, it is the following:

import { dispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { CustomerEffortScore } from '@woocommerce/customer-effort-score';
import { uniqueId } from 'lodash';

const CustomerEffortScoreWithCustomNotice = ( {
	customOptions = {},
	...rest
} ) => {

        // core/notices2 was used in @woocommerce/customer-effort-score so I left it as it was.
	const { createNotice, removeNotice } = dispatch( 'core/notices2' );

	const idRef = useRef( customOptions.id || uniqueId() );

	//remove notice when the component is unmounted
	useEffect( () => () => removeNotice( idRef.current ), [] );

	return (
		<CustomerEffortScore
			{ ...rest }
			createNotice={ ( type, label, options ) =>
				createNotice( type, label, {
					...options,
					...customOptions,
					id: idRef.current,
				} )
			}
		/>
	);
};

When CustomerEffortScorePrompt is unmounted, we could use removeNotice, to dismiss the prompt/snackbar.
For this we need the notice ID, unfortunately <CustomerEffortScore> component creates the ID randomly and it does not give the option to set the ID externally.
As the default <CustomerEffortScore> component does not give the option to set a custom ID, we create a custom notice so we are able to dismiss the notice if the component is unmounted.

The behaviour will be similar than it was before, except that the prompt will be only visible if it is mounted in the current view, so if the user changes the page, the prompt will be gone as the component it is not mounted anymore.

What do you think?

Jun 07 '22 19:06 jorgemd24

@jorgemd24

Does the prompt need to be shown on every page? or only in the dashboard/product feed page?

Hmm, that's a good question. I've just read through the original feature request in #882 and still cannot have a conclusion about it. Basically the requirements are:

CES prompt for setting up GLA

When the user goes to the product page for the first time, it will show the success modal. When the success modal is closed, the CES prompt will show up.

CES prompt for creating a Google Ad campaign

When the user create an Ad campaign, it will show the campaign creation success modal. When the user clicks Got it on the success modal it will be closed and the CES prompt will show up.

In my view if the CES prompt shows up and the user did not click on it and they go to other pages instead, the CES prompt should be closed automatically. Simply because I think the user doesn't care about the prompt and would pretty likely to just click the close (X) button of it.

@j111q What do you think? Do both prompts need to be shown on every page, or only in the dashboard / product feed pages?

Jun 08 '22 08:06 ianlin

@jorgemd24 ,

I think the easiest way to have the CustomerEffortScore working as "React", so when CustomerEffortScore is unmounted, the notice dissapered too, it is the following: [...]

In your code example, we provide createNotice prop to CustomerEffortScore component, but there isn't a createNotice prop? 🤔 Source: https://github.com/woocommerce/woocommerce/blob/be15a3503837dabe74d78407810ed426f819b3ec/packages/js/customer-effort-score/src/customer-effort-score.tsx#L25-L46

I think your idea there is pretty much in the right direction, as in we use our own createNotice and removeNotice. I'm thinking, instead of using CustomerEffortScore component, we use the CustomerFeedbackModal component, like how it is used in CustomerEffortScore component, but without the problem of createNotice and useEffect in CustomerEffortScore.

@ianlin ,

Does the prompt need to be shown on every page? or only in the dashboard/product feed page?

In my view if the CES prompt shows up and the user did not click on it and they go to other pages instead, the CES prompt should be closed automatically. Simply because I think the user doesn't care about the prompt and would pretty likely to just click the close (X) button of it.

I have mixed thoughts about this. On one hand, I think your reason is valid. On the other hand, I also think it may feel buggy, like "I switched to other pages / other tabs, why did it dismiss the notice at the bottom of the page? I wanted to give feedback after looking at the other tabs, and now I can't give feedback anymore because the notice is dismissed when I switched tabs." 🤔

I'll leave this design decision to @j111q . 😄

Jun 08 '22 11:06 ecgan

Thanks @ecgan

In your code example, we provide createNotice prop to CustomerEffortScore component, but there isn't a createNotice prop? 🤔 Source: https://github.com/woocommerce/woocommerce/blob/be15a3503837dabe74d78407810ed426f819b3ec/packages/js/customer-effort-score/src/customer-effort-score.tsx#L25-L46

Ah! It seems that createNotice prop has been removed when it was migrated from JS to TS. See PR 33110

In the previous versions and, in my local package, the CustomerEffortScore has a createNotice prop.

I think your idea there is pretty much in the right direction, as in we use our own createNotice and removeNotice. I'm thinking, instead of using CustomerEffortScore component, we use the CustomerFeedbackModal component, like how it is used in CustomerEffortScore component, but without the problem of createNotice and useEffect in CustomerEffortScore.

Thanks for this, I am working on it and as well for a reusable component to create notices that can be removed when the component is unmount.

I have mixed thoughts about this. On one hand, I think your reason is valid. On the other hand, I also think it may feel buggy, like "I switched to other pages / other tabs, why did it dismiss the notice at the bottom of the page? I wanted to give feedback after looking at the other tabs, and now I can't give feedback anymore because the notice is dismissed when I switched tabs."

If we would like to show on every page, we will need to think about how to render the component globally in the app. I am not sure if we already have something similar.

Jun 08 '22 20:06 jorgemd24

@jorgemd24 ,

Ah! It seems that createNotice prop has been removed when it was migrated from JS to TS. See PR 33110

In PR #33110, the createNotice prop was already not there in the CustomerEffortScore component, the devs removed supplying the prop in test in https://github.com/woocommerce/woocommerce/pull/33110/files#diff-d50cc71cacb845f1663f3426683cb4f488ca1fbeff99185706c218ae77599e56L64-L68.

I looked deeper and the prop was actually removed from the component in https://github.com/woocommerce/woocommerce/pull/32538/files#diff-68a3f6fa9ff5ed6b1718a40d9ca3188b7fd0dba8a816462c94f7a456d5451384R30-R37.

If we would like to show on every page, we will need to think about how to render the component globally in the app. I am not sure if we already have something similar.

I believe we don't have that. If we want to show on every page nicely, I think we need to have the AppShell implementation (issue https://github.com/woocommerce/google-listings-and-ads/issues/1548).

Jun 09 '22 08:06 ecgan

@ecgan, @ianlin

I have updated the code following the previous discussion: https://github.com/woocommerce/google-listings-and-ads/pull/1543#issuecomment-1149791189.

With these changes we are:

Removing the prompt when the component is unmounted.
We are fixing the issue of showing multiple notices
We are fixing the issue that @ianlin mentioned before: https://github.com/woocommerce/google-listings-and-ads/pull/1543#issuecomment-1144518273

There are some points that need to be discussed for the Dashboard prompt:

If the prompt is showing and the user goes to a subpath (for example edit campaign), and then it goes back to the dashboard, should we re-render the prompt in the dashboard?
Should the prompt be shown on every page? Outside of the dashboard/product feed?

The tests are failing, because of the bundle size, I am not sure why this is happening but I think it is related to importing CustomerFeedbackModal component, any idea?

Jun 10 '22 13:06 jorgemd24

@jorgemd24 @ianlin ,

The tests are failing, because of the bundle size, I am not sure why this is happening but I think it is related to importing CustomerFeedbackModal component, any idea?

This is interesting. I looked into the issue and I think I have the explanation for it.

Prior to @jorgemd24's updated code (as mentioned in https://github.com/woocommerce/google-listings-and-ads/pull/1543#issuecomment-1152381803), bundle size check was running okay, but now it fails with the updated code.

Before the updated code, we were using import @woocommerce/customer-effort-score. In the updated code, we are using @woocommerce/customer-effort-score/build/customer-feedback-modal.

In GLA, we are using this https://github.com/woocommerce/woocommerce/tree/trunk/packages/js/dependency-extraction-webpack-plugin webpack plugin (aka. DEWP):

https://github.com/woocommerce/google-listings-and-ads/blob/29a724b2c4e2dd770c29f12d1035a60ae8263ead/webpack.config.js#L4

This means that when we have a package import that matches the defined list of packages (including @woocommerce/customer-effort-score; see https://github.com/woocommerce/woocommerce/blob/1e03b68598ef6e9758d09c61424bf4b730d26fc5/packages/js/dependency-extraction-webpack-plugin/assets/packages.js for the complete list), the package will not be bundled, instead GLA will use the package available in the global scope injected by WC. See example screenshot of the global wc.customerEffortScore below:

The "before" code was passing bundlesize check because it does not have the @woocommerce/customer-effort-score bundled. In the updated code, because @woocommerce/customer-effort-score/build/customer-feedback-modal does not match the defined list in DEWP, it is bundled together, causing bundlesize check to fail. We can run npm run env -- NODE_ENV=production wp-scripts build --webpack-bundle-analyzer and see that it is bundled in a visualization like below (I learned this from Eason's PR https://github.com/woocommerce/google-listings-and-ads/pull/1073).

After discovering the above, the next question is: why do we have to import from @woocommerce/customer-effort-score/build/customer-feedback-modal? Why can't we import from @woocommerce/customer-effort-score, which should then solve the bundle size issue?

When I first suggested using CustomerFeedbackModal in https://github.com/woocommerce/google-listings-and-ads/pull/1543#issuecomment-1149791189, I looked into the package index.ts file https://github.com/woocommerce/woocommerce/blob/5db5c8b110412556806b54b34ecd8ba513db1387/packages/js/customer-effort-score/src/index.ts, and it does export CustomerFeedbackModal.

Now, I looked deeper into it, and I realized that the component export was just done recently (see PR https://github.com/woocommerce/woocommerce/pull/32538/files#diff-22de08ccb210a9adf3a9b8b57c9be9fd58bafb47212e1b64ea0e7c7f7677373b), and it has not been published on npm yet. In npm, the latest version is 2.0.1 published 3 months ago. In GLA, we are using version 1.1.0 published 10 months ago. That's why we don't have the CustomerFeedbackModal exported from the package.

So, to solve this issue, I think we would want to get the relevant team to publish a new version for the @woocommerce/customer-effort-score package, and then we use that new version in GLA, then everything should work nicely. You can also test by changing @woocommerce/customer-effort-score version in GLA package.json and make it point to your local git checkout copy of https://github.com/woocommerce/woocommerce/tree/trunk/packages/js/customer-effort-score directory in your machine (so GLA doesn't use the published npm package) and see if the idea works.

Jun 13 '22 18:06 ecgan

Hey, I'd like to chip in some suggestions. 🙏

About the cause of bloat bundle size

The main problem is that @woocommerce/customer-effort-score didn't build its ES Module correctly when publishing version 1.1.0.

Incorrect build for ES Modules

From the node module directory, we can find it only has the build of CommonJS Module in the build directory but no build-module. That's why it needs to use import CustomerFeedbackModal from '@woocommerce/customer-effort-score/build/customer-feedback-modal'; in this PR.

2022-06-14 11 12 48

DEWP and GLA's webpack config

As @ecgan mentioned in https://github.com/woocommerce/google-listings-and-ads/pull/1543#issuecomment-1154241021, @woocommerce/customer-effort-score/build/customer-feedback-modal will be bundled since it doesn't match the packages provided by DEWP.

The dependent packages in <CustomerFeedbackModal> will be provided via DEWP as well if it matches. When webpack resolves the @wordpress/components, we state it should be bundled in GLA wepback config. Ideally, it should not lead to the bundle size problem since GLA has already bundled @wordpress/components from the beginning.

The root cause

Originally, it should use the same bundled @wordpress/components as other codes are dependent on, but the problem is the wrong build of the module type. When compiling, webpack couldn't optimize the bundle size via tree shacking if the package only provides the build of CommonJS Module. So in the end, all <CustomerFeedbackModal> dependencies are bundled into the compiled result. That's the root cause of why its size bloats so much.

Ref: the warning block in https://webpack.js.org/guides/author-libraries/#final-steps

Possible workarounds

Ideally, the solution will be using the version that officially exports the <CustomerFeedbackModal> or asking upstream to make changes to the showing/closing behavior of the notice. But considering the L-2 support policy, we probably need to wait for several months till the L-2 hits the WC 6.7.

So currently, we have two possible workarounds that need a smaller change scope.

Workaround A

Create another PR that branches off develop, and remove the notice in the <CustomerEffortScorePrompt> component. Adding code snippets as below to the component allows us to experiment with the feasibility. And the bundle size of index.js is ≈ 197.6 KB.

useEffect( () => {
  return () => {
    const notice = wp.data
      .select( 'core/notices2' )
      .getNotices()
      .find( ( el ) => el.content === label );

    if ( notice ) {
      wp.data.dispatch( 'core/notices2' ).removeNotice( notice.id );
    }
  };
}, [ label ] );

(💡 It's for experiment only. The selector and action of 'core/notices2' should be used in a similar way to useUnmountableNotice in this PR.)

And this PR could be set on hold till a release version of WC exports <CustomerFeedbackModal> and hits the L-2 policy. I guess this PR might be reused directly as the better solution then.

Workaround B

It's about to release the WC 6.6 in a few days, so the L-2 of WC could be bumped to 6.4, which uses WC-admin 3.3.2. And @woocommerce/customer-effort-score in that WC-admin is version 2.0.0, which has fixed the incorrect build of module types.

So we could bump the @woocommerce/customer-effort-score to 2.0.0 and use the correct ES Module build in this PR as below. The bundle size of index.js is ≈ 222.0 KB.

import CustomerFeedbackModal from '@woocommerce/customer-effort-score/build-module/customer-feedback-modal';

Suggestion

Comparing the two workarounds, both rely on internal implementation and might be incompatible someday. But the workaround A has a smaller bundle size. IMO, I would suggest the workaround A for now. And we could still have a better solution by holding this PR and waiting for the WC 6.7 to hit the L-2.

Jun 14 '22 04:06 eason9487

Sorry for late reply, just reading through this thread ☝️

If the prompt is showing and the user goes to a subpath (for example edit campaign), and then it goes back to the dashboard, should we re-render the prompt in the dashboard?

What does "re-render the prompt" mean? Does it mean, for example, you hide the first existing prompt and show a new second prompt? I don't really have a preference on whether the user is seeing the first prompt or a second prompt -- I think ultimately they should only be seeing one prompt at a time.

Does the prompt need to be shown on every page? or only in the dashboard/product feed page?

In my view if the CES prompt shows up and the user did not click on it and they go to other pages instead, the CES prompt should be closed automatically. Simply because I think the user doesn't care about the prompt and would pretty likely to just click the close (X) button of it.

I have mixed thoughts about this. On one hand, I think your reason is valid. On the other hand, I also think it may feel buggy, like "I switched to other pages / other tabs, why did it dismiss the notice at the bottom of the page? I wanted to give feedback after looking at the other tabs, and now I can't give feedback anymore because the notice is dismissed when I switched tabs." 🤔

^ I think it makes sense that the notice doesn't automatically disappear, even if you go to another page. Let's allow the user to close it if they want to close it, otherwise it can stay there.

I tested around and it gives me another question: we are already in product feed page, why do we need to open a new tab that shows product feed page again? I'm thinking it is because we want to retain the modal in the first tab so that users can click on the next button, but from the user experience perspective, it feels weird (and maybe even appear buggy) to open a new tab showing the same page.

Happy to share that @dominiquevias is working on a better design here, and in an upcoming iteration, we will NOT be opening the product feed in a new tab from the modal. 😊

Jun 14 '22 06:06 j111q

Thanks, @eason9487 and @ecgan for your comments,

Workaround A Create another PR that branches off develop, and remove the notice in the <CustomerEffortScorePrompt> component. Adding code snippets as below to the component allows us to experiment with the feasibility. And the bundle size of index.js is ≈ 197.6 KB.

I agree with you, that for now, this is the best workaround. I am preparing the PR and implementing it. When the customer effort package is updated we can look back to this PR and see which options are better.

@j111q, thanks for your input!

What does "re-render the prompt" mean? Does it mean, for example, you hide the first existing prompt and show a new second prompt? I don't really have a preference on whether the user is seeing the first prompt or a second prompt -- I think ultimately they should only be seeing one prompt at a time.

The "re-render" is more from the React point of view but means exactly what you explained. The thing is that for now, we do not have a way to show the prompt on every page with its CES modal, so the prompt will disappear when it goes to a subpath and it will show again if it goes to the Dashboard, this will only happen in the dashboard CES prompt.

I think ultimately they should only be seeing one prompt at a time.

Yes, this will be fixed. Before this PR, it was showing duplicated notices.

We are thinking to implement an AppShell (see https://github.com/woocommerce/google-listings-and-ads/issues/1548) to be able to show the same component, on every page in the GLA app without the need to remove and display the component every time that the page changes.

Jun 14 '22 14:06 jorgemd24