Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543

jorgemd24 · 2022-05-31T18:49:18Z

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.

google-listings-and-ads/js/src/dashboard/index.js

Line 35 in 74949bc

const [ isCESPromptOpen, setCESPromptOpen ] = useState( false );

Before this PR

Screen.Capture.on.2022-05-31.at.20-37-57.mp4

After this PR

Screen.Capture.on.2022-05-31.at.20-43-05.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

ecgan

Tested the code before and after the PR code change, I can reproduce the issue, and the code change does fix the issue.

I left one 💅 comment below, a suggestion to add code comment to explain why the useEffect is needed.

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. This approach would also solve the issue where the notice appears to keep re-appearing when we switch tabs, see demo video of the issue below:

issue-notice-reappearing-switch-tabs.mov

I also looked into the CustomerEffortScorePrompt, and the more I look into it, the deeper rabbit hole it gets, and the more questions I have: e.g.

Why do we need the use of localStorage? It is introduced in PR Add CES prompts for initial setup and campaign creation #1152.
I found the reason in commit 0ff1b0f (#1152), and it is because we want to have control of state between two window tabs.
Based on the commit message, 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. See demo video below:

issue-new-tab-same-product-feed-page.mov

In the end, since the code here fixes the issue, I'm approving this PR. I think it would be good to get a second review from @ianlin too, since he worked on the PR #1152 above, he might have a bit more insight and ideas that I miss. 😄 🙏

ecgan · 2022-06-01T18:22:25Z

js/src/dashboard/index.js

+	useEffect( () => {
+		setCESPromptOpen( false );
+	}, [ query.subpath ] );


💅 Can we add in a code comment to explain why this is needed? Something like: "When users navigate into one of the subpaths (edit free listings, edit paid ads, and create paid ads), we call setCESPromptOpen( false ) via effect, so that when users come back into dashboard, the CustomerEffortScorePrompt component will not be rendered and will not trigger another notice. There will be only one notice throughout."

ianlin · 2022-06-02T07:08:23Z

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:

Screen.Recording.2022-06-02.at.15.02.42.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.

jorgemd24 · 2022-06-02T12:04:25Z

@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):

Screen.Capture.on.2022-06-02.at.12-55-56.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!

ecgan · 2022-06-02T16:48:21Z

@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 #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.

jorgemd24 · 2022-06-07T19:47:33Z

@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 React component that will display a notice when we render the component, but if we use the "React way of doing things", when 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:

Screen.Capture.on.2022-06-07.at.21-43-21.mp4

Screen.Capture.on.2022-06-07.at.21-40-36.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?

ianlin · 2022-06-08T08:24:56Z

@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?

ecgan · 2022-06-08T11:23:52Z

@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 . 😄

jorgemd24 · 2022-06-08T20:37:12Z

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.

ecgan · 2022-06-09T08:29:11Z

@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 #1548).

jorgemd24 · 2022-06-10T13:47:18Z

@ecgan, @ianlin

I have updated the code following the previous discussion: #1543 (comment).

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: Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543 (comment)

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?

ianlin

Thanks, it tested as expected on my end: the CES notice will disappear when user goes to a sub-page, and it will show again when the user goes back to the main page, and clicks on the Give feedback will show the feedback modal.

For the two question I think we still need @j111q's input:

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?

For the bundle size, I also think it's related to importing '@woocommerce/customer-effort-score/build/customer-feedback-modal', but I don't have any clues at the moment 😓.

ianlin · 2022-06-13T07:31:25Z

js/src/hooks/useCESNotice.js

+	return useUnmountableNotice( 'success', label, {
+		actions: [
+			{
+				label: __( 'Give feedback', 'woocommerce-admin' ),


Should it be google-listings-and-ads rather than woocommerce-admin?

Thanks @ianlin, it is updated: f674ca7

For the bundle size, I also think it's related to importing '@woocommerce/customer-effort-score/build/customer-feedback-modal', but I don't have any clues at the moment

It is related to @wordpress/components, I think the issue is that @woocommerce/customer-effort-score/build/customer-feedback-modal uses a different version of @wordpress/components, and this increases the bundle size.

I am looking into possible solutions, maybe @ecgan have seen this issue before.

I am looking into possible solutions, maybe @ecgan have seen this issue before.

See my comment #1543 (comment) below.

ecgan · 2022-06-13T18:25:53Z

@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 #1543 (comment)), 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):

google-listings-and-ads/webpack.config.js

Line 4 in 29a724b

    
           const WooCommerceDependencyExtractionWebpackPlugin = require( '@woocommerce/dependency-extraction-webpack-plugin' );

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 #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 #1543 (comment), 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.

eason9487 · 2022-06-14T04:44:21Z

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.

DEWP and GLA's webpack config

As @ecgan mentioned in #1543 (comment), @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.

j111q · 2022-06-14T06:49:16Z

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. 😊

jorgemd24 · 2022-06-14T14:38:48Z

Thanks, @eason9487 and @ecgan for your comments,

Workaround A
Create another PR that branches off develop, and remove the notice in the 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 #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.

Add useEffect to avoid multiple CES prompts

74949bc

jorgemd24 self-assigned this Jun 1, 2022

jorgemd24 requested a review from a team June 1, 2022 14:49

jorgemd24 marked this pull request as ready for review June 1, 2022 14:50

jorgemd24 changed the title ~~Add useEffect to avoid multiple CES prompts in the Dashboard Page~~ Add useEffect to avoid multiple CES prompts on the Dashboard Page Jun 1, 2022

ecgan approved these changes Jun 1, 2022

View reviewed changes

ecgan mentioned this pull request Jun 2, 2022

UI AppShell containing the whole plugin #1548

Closed

jorgemd24 added 10 commits June 9, 2022 22:13

Add hook useUnmountableNotice

4ca8ac0

Add test hook unmountableNotice

c62f084

Add CESNotice

b0e9e9f

Add CESNotice tests

1ac0a90

Refactor CustomerEffortScore component

28a819b

Add tests for CustomerEffortScoreUnmountableNotice

5a00fcf

Replace CustomerEffortScore with CustomerEffortScoreUnmountableNotice

1e488d9

Remove useEffect to remove CES prompt in the dashboard component

dd18a9e

Update CustomerEffortScoreUnmountableNotice description

3a77cb4

Revert changes in dashboard.js - getQuery

a22b042

ianlin reviewed Jun 13, 2022

View reviewed changes

Update Give feedback domain CES Notice.

f674ca7

jorgemd24 mentioned this pull request Jun 14, 2022

Fix - CES Prompts Showing Several Times #1564

Merged

tomalec added the type: technical debt - dependency extraction This issue/PR suffers from the dependency extraction and management label Jun 14, 2022

jorgemd24 added the status: on hold The issue is currently not prioritized. label Jun 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543

Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543

jorgemd24 commented May 31, 2022 •

edited

ecgan left a comment

ecgan Jun 1, 2022

ianlin commented Jun 2, 2022

jorgemd24 commented Jun 2, 2022 •

edited

ecgan commented Jun 2, 2022

jorgemd24 commented Jun 7, 2022 •

edited

ianlin commented Jun 8, 2022

ecgan commented Jun 8, 2022

jorgemd24 commented Jun 8, 2022

ecgan commented Jun 9, 2022

jorgemd24 commented Jun 10, 2022

ianlin left a comment

ianlin Jun 13, 2022

jorgemd24 Jun 13, 2022

ecgan Jun 13, 2022

ecgan commented Jun 13, 2022

eason9487 commented Jun 14, 2022 •

edited

j111q commented Jun 14, 2022 •

edited

jorgemd24 commented Jun 14, 2022

Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543

Are you sure you want to change the base?

Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543

Conversation

jorgemd24 commented May 31, 2022 • edited

Changes proposed in this Pull Request:

Before this PR

After this PR

Detailed test instructions:

Changelog entry

ecgan left a comment

Choose a reason for hiding this comment

ecgan Jun 1, 2022

Choose a reason for hiding this comment

ianlin commented Jun 2, 2022

jorgemd24 commented Jun 2, 2022 • edited

ecgan commented Jun 2, 2022

jorgemd24 commented Jun 7, 2022 • edited

ianlin commented Jun 8, 2022

CES prompt for setting up GLA

CES prompt for creating a Google Ad campaign

ecgan commented Jun 8, 2022

jorgemd24 commented Jun 8, 2022

ecgan commented Jun 9, 2022

jorgemd24 commented Jun 10, 2022

ianlin left a comment

Choose a reason for hiding this comment

ianlin Jun 13, 2022

Choose a reason for hiding this comment

jorgemd24 Jun 13, 2022

Choose a reason for hiding this comment

ecgan Jun 13, 2022

Choose a reason for hiding this comment

ecgan commented Jun 13, 2022

eason9487 commented Jun 14, 2022 • edited

About the cause of bloat bundle size

Incorrect build for ES Modules

DEWP and GLA's webpack config

The root cause

Possible workarounds

Workaround A

Workaround B

Suggestion

j111q commented Jun 14, 2022 • edited

jorgemd24 commented Jun 14, 2022

jorgemd24 commented May 31, 2022 •

edited

jorgemd24 commented Jun 2, 2022 •

edited

jorgemd24 commented Jun 7, 2022 •

edited

eason9487 commented Jun 14, 2022 •

edited

j111q commented Jun 14, 2022 •

edited