-
Notifications
You must be signed in to change notification settings - Fork 274
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
Create Ads Module Setup CTA Banner #8660
Comments
Looks good. I'm guessing we don't have any GA events defined yet for this banner, but we should probably add those as well in a follow-up issue 😅 (If we get them in soon we can probably just add it to this issue's ACs.) ACs look good, moving to IB. |
I've created the IB today, one thing to note and discuss in the IBR is that we could quite easily have the Consent Mode and Ads Module widgets both rendering on the dashboard. We could prevent this as a one off by doing some kind of check for the Consent Mode display state within this new Widget but that's not very flexible and brings in CoMo logic to an Ads module widget, although perhaps this is acceptable for now? We could perhaps create a follow up ticket to create a higher level component for these banners that handles the checks and only renders one at a time regardless if multiple widgets meet their display condition. I'm also waiting on details of the responsive designs from @sigal-teller here. |
Putting this into IBR, this comment from above still applies and is worth discussing:
|
Thanks, @benbowler. Mostly looks good to me.
We need to use the same approach that we use in the settings when we click on the connect button: site-kit-wp/assets/js/components/settings/SetupModule.js Lines 60 to 69 in e626cfc
Yes, let's not worry about that for now. We will create a follow up task if it is needed. |
Thanks @eugene-manuilov, I've updated the IB module activation steps. |
Thanks, @benbowler. IB ✔️ |
QA Update ❌
@zutigrm Please find my observations below 1) Font family for the banner title is 'Google Sans Display' in Figma where as under actual implementation it is 'Google Sans Text'. 2) We don't have figma design for large tablet viewports but design under Viewports having width between 961px to 1132px is looking off with space appearing below the image. 3) In smaller tablet viewports font size is different from figma. On smaller tablets we implemented same font size as mobile viewports. Tested on iPad air and Surface Pro smaller tablet viewports. 4) The 'Maybe later' CTA is not functioning as expected according to the acceptance criteria and QA brief. Clicking on the 'Maybe later' CTA does not dismiss the banner but instead renames the CTA to "Don’t show again". Clicking on 'Don’t show again' dismisses the banner. Additionally, upon each refresh when the dashboard loads, the Ads module setup CTA banner briefly displays for a few milliseconds before hiding. Recording.993.mp4 |
QA Update
|
@mohitwp Thanks for checking it. Hm, I tested it manually on my end, as sometimes reference date doesn't affect some places. Which seems to be the case. You can edit the database, in You will see |
QA Update ❌
@zutigrmI tested this on three different sites. According to the acceptance criteria, after clicking 'Maybe later', the CTA banner should display again in 14 days. In the database, the expiry date is correctly set to 14 days later. However, when I set a past date in the database, it forces the banner to display on the dashboard, but it still shows the 'Maybe later' CTA. It only displays the 'Don't show' CTA after appearing twice with the 'Maybe later' CTA. Expected: On the second appearance, the banner should display the 'Don't show' CTA instead of 'Maybe later'. Recording.1009.mp4 |
QA Update ✅
1) Font family for the banner title is 'Google Sans Display' in Figma where as under actual implementation it is 'Google Sans Text'. - PASS
3) In smaller tablet viewports font size is different from figma. On smaller tablets we implemented same font size as mobile viewports. Tested on iPad air and Surface Pro smaller tablet viewports. - PASS 4) The 'Maybe later' CTA is not functioning as expected according to the acceptance criteria and QA brief. Clicking on the 'Maybe later' CTA does not dismiss the banner but instead renames the CTA to "Don’t show again". Clicking on 'Don’t show again' dismisses the banner. Additionally, upon each refresh when the dashboard loads, the Ads module setup CTA banner briefly displays for a few milliseconds before hiding. PASS 'Maybe later' CTA Recording.1005.mp4'Don't Show' CTA Recording.1012.mp4 |
Feature Description
With the Ads module now released, there is a need to create an Ads setup banner with applicable CTA that prompts the users to set up the module should they not yet have it set up. This setup banner will function like all prior setup banners and will contain a CTA to facilitate module set up, or be dismissed.
The Figma design can be viewed here. Following is a screenshot of the final setup banner design:
Note, copy is not final at the time of writing, and will likely be altered prior to execution. This issue will be updated to reflect the final copy once it is approved.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
assets/js/modules/ads/components/dashboard/AdsModuleSetupCTAWidget.js
ADS_MODULE_SETUP_BANNER_PROMPT_DISMISSED_KEY = "ads_module_setup_banner_prompt_dismissed_key"
isModuleActive
selector onCORE_MODULES
to check if the ads module is already active, if it is active, return null from the component so that it is not rendered on the dashboard.assets/sass/components/ads/_googlesitekit-ads-setup-banner.scss
based on the latest FIGMA designuseBreakpoint
hook to conditionally render the three different SVGs for each breakpoint; desktop, tablet and mobile.Button
component with which calls anonSetup
callback function:activateModule
selector to get themoduleReauthURL
, then usenavigateTo
to go to the module setup, as we do in theSetupModule
onSetup functiontrackEvent
here to track anactivate_module
event, as we do in theSetupModule
onSetup functionisPromptDismissed
passing the slugADS_MODULE_SETUP_BANNER_PROMPT_DISMISSED_KEY
.getPromptDismissCount
passing the slugADS_MODULE_SETUP_BANNER_PROMPT_DISMISSED_KEY
.handleDismissClick
function and add it to theonClick
event of this buttonawait dismissPrompt( ADS_MODULE_SETUP_BANNER_PROMPT_DISMISSED_KEY, { expiresInSeconds: twoWeeksInSeconds});
dismissPrompt( ADS_MODULE_SETUP_BANNER_PROMPT_DISMISSED_KEY );
with no second argument.site-kit-wp/assets/js/components/DashboardEntityApp.js
Line 225 in 9555a23
Test Coverage
QA Brief
Maybe Later
is clicked, banner should not show again. So, set the reference date in tester plugin to something over 14 days, 15days or 1 month for example. And verify that banner is showing again with second CTA having text "Don’t show again" and after it is clicked, banner is permanently dismissedChangelog entry
The text was updated successfully, but these errors were encountered: