-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add useEffect to avoid multiple CES prompts on the Dashboard Page #1543
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 😄 🙏
js/src/dashboard/index.js
Outdated
useEffect( () => { | ||
setCESPromptOpen( false ); | ||
}, [ query.subpath ] ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 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."
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.movI 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 🐛 😅.
I searched a bit from the previous commits and found it's a requirement in #555, quotes from @j111q:
|
Thanks for your suggestions 🙂 , related to:
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:
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.
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 Screen.Capture.on.2022-06-02.at.12-55-56.mp4
Thanks for your comments, I think I know what is the problem:
Package:
This issue will affect the current solution too, I will see how we can fix this as well. Let me know your thoughts! |
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 haven't looked detailed into the code there yet, but I have a feeling that An idea to address the above is we can probably come up with a wrapper around |
Yes, currently (using develop/trunk) the prompt is throwing an error when you are changing the page and clicking on "Give feedback".
See the following videos: Screen.Capture.on.2022-06-07.at.21-43-21.mp4Screen.Capture.on.2022-06-07.at.21-40-36.mp4@ianlin I have been playing with
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? |
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
CES prompt for creating a Google Ad campaign
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? |
In your code example, we provide I think your idea there is pretty much in the right direction, as in we use our own @ianlin ,
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 . 😄 |
Thanks @ecgan
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.
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.
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. |
In PR #33110, the I looked deeper and the prop was actually removed from the component in https://github.com/woocommerce/woocommerce/pull/32538/files#diff-68a3f6fa9ff5ed6b1718a40d9ca3188b7fd0dba8a816462c94f7a456d5451384R30-R37.
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). |
I have updated the code following the previous discussion: #1543 (comment). With these changes we are:
There are some points that need to be discussed for the Dashboard prompt:
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😓.
js/src/hooks/useCESNotice.js
Outdated
return useUnmountableNotice( 'success', label, { | ||
actions: [ | ||
{ | ||
label: __( 'Give feedback', 'woocommerce-admin' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be google-listings-and-ads
rather than woocommerce-admin
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking into possible solutions, maybe @ecgan have seen this issue before.
See my comment #1543 (comment) below.
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 In GLA, we are using this https://github.com/woocommerce/woocommerce/tree/trunk/packages/js/dependency-extraction-webpack-plugin webpack plugin (aka. DEWP):
This means that when we have a package import that matches the defined list of packages (including The "before" code was passing bundlesize check because it does not have the After discovering the above, the next question is: why do we have to import from When I first suggested using 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 So, to solve this issue, I think we would want to get the relevant team to publish a new version for the |
Hey, I'd like to chip in some suggestions. 🙏 About the cause of bloat bundle sizeThe main problem is that Incorrect build for ES ModulesFrom the node module directory, we can find it only has the build of CommonJS Module in the DEWP and GLA's webpack configAs @ecgan mentioned in #1543 (comment), The dependent packages in <CustomerFeedbackModal> will be provided via DEWP as well if it matches. When webpack resolves the The root causeOriginally, it should use the same bundled Ref: the warning block in https://webpack.js.org/guides/author-libraries/#final-steps Possible workaroundsIdeally, 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 ACreate another PR that branches off 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 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 BIt'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 So we could bump the import CustomerFeedbackModal from '@woocommerce/customer-effort-score/build-module/customer-feedback-modal'; SuggestionComparing 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. |
Sorry for late reply, just reading through this thread ☝️
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.
^ 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.
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. 😊 |
Thanks, @eason9487 and @ecgan for your comments,
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!
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.
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. |
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 alwaystrue
when navigating through the dashboard sub-components such asEditFreeCampaign
,EditPaidAdsCampaign
andCreatePaidAdsCampaign
, 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
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:
wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fdashboard&guide=campaign-creation-success
Changelog entry