site-kit-wp: Unavailable notice for Adsense "Top Earning Pages" widget

Feature Description

Since the Adsense Top Earning Pages widget works based on a connection between Adsense and UA, and such a connection is not possible at this moment with GA4, it should be replaced with an unavailable notice in the GA4 dashboard view, according to the Figma designs.

Here is a screenshot for visual aid: image

For this notice, the same component created in #6556 can be re-purposed.

Here is the relevant section in the design doc.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • When GA4 Reporting is enabled and AdSense is connected and linked to GA, the “Top Earnings Pages” widget should be replaced with the notice in the Figma design.
  • The notice should contain an “OK, Got it!” button (different from the figma designs, so it’s in-line with our existing “OK, Got it!” translation string).
    • When clicked, this button should permanently dismiss the banner. This should cause the widget to return <WidgetNull />, meaning the widget area should disappear if no other widgets appear in the area.
  • The header in the banner should be: "Top earning pages are not yet available in Google Analytics 4."
  • The body text in the banner should be: "Site Kit will notify you as soon as you can connect AdSense and Analytics again."

Implementation Brief

  • In assets/js/components/settings/SettingsNotice/SettingsNotice.js:
    • To avoid duplicating dismissal logic for notices, adapt this component to accept a dismiss (string) prop. If a string is passed, use it as the “dismissal key” (see below), and render an “OK, Got it!” button vertically aligned with the whole component.
    • When the dismiss button (“OK, Got it!”) is clicked, it should use the dismissItem action from the core/user store with the dismissal prop to persistently store the dismissal in the DB. This functionality can be copied from the EnableAutoUpdateBannerNotification component.
  • In assets/js/modules/adsense/constants.js, create a new ADSENSE_GA4_TOP_EARNING_PAGES_NOTICE_DISMISSED_ITEM_KEY that will be used to persistently store the notice dismissal.
  • In assets/js/modules/adsense/components/dashboard, create and export a new functional component DashboardTopEarningPagesWidgetGA4.
    • This component should accept the <WidgetNull> prop.
    • Use the isItemDismissed with the above dismissed item key to check if this notice is dismissed. If it is, return <WidgetNull>.
    • If the notice is not dismissed, render the SettingsNotice component passing the header in the AC as the notice. The body text should be passed as the children within this component and it will render this below the notice. Some design tweaks might be necessary. Ensure the existing usage of the <SettingsNoticeMultiRow> is not affected too much.
    • NOTE: For now, use the above existing component without styling it as per the Figma mock. The styling should be consistently updated for ALL notices in #6702.
  • In assets/js/modules/adsense/index.js, refactor the Component key when registering the adsenseTopEarningPages widget. Use the <GA4DashboardWidgetSwitcher> to pass the existing DashboardTopEarningPagesWidget as the UA component and the new DashboardTopEarningPagesGA4NoticeWidget component as the GA4 variant. See #6550 for usage.
  • Add a Storybook story and VRT scenario for this GA4 notice variant widget.

Test Coverage

  • No new tests required.

QA Brief

  • Make sure GA4 Reporting is enabled and AdSense is connected and linked to GA
  • Check the Performance over the last 28 days section - the notice should appear there Screenshot 2023-04-06 at 17 15 17

Changelog entry

  • Add the new GA4 version of the Top Earning Pages widget.

About this issue

  • Original URL
  • State: closed
  • Created a year ago
  • Comments: 16

Most upvoted comments

@sashadoes I see you set QAB as not applicable for this ticket. Why is that? Could you please update QAB to have the correct steps to QA your changes? Thanks.

It might still be possible to use the <ReportTable> but I don’t think it’s a critical detail for a placeholder component like that. It’s worth looking into, but mostly something we don’t want this issue to get held up by.

Thanks @eugene-manuilov – I just spoke to @sashadoes about this – I think the important part here is the addition of the wrapping <Widget>, whether or not it uses an enhanced <ReportTable> here is a bit secondary – if it’s possible sure but we probably don’t need to invest too much effort to match that aspect of the design exactly.

@eugene-manuilov Thanks for the feedback. Updated the IB with the appropriate component name!

My main concern here is that it won’t be clear which notice it is because we already have quite a lot different notices and having one more that just says “Notice” might be confusing.

That is a lot of notices! I only saw the word “Notices” in our Storybook and thought just removing the word Settings from the component name would be fine. AlertNotice sounds alright though. We can deal with this in the IB for #6702.

Thanks again!

@jimmymadon

Let’s rename the SettingsNotice component to be Alert and move it to the components folder.

I would suggest renaming this component to Notice as “Alert” sounds a bit more like a JS Alert popup. I’ve added a note to #6702 as well to include the refactoring there.

My main concern here is that it won’t be clear which notice it is because we already have quite a lot different notices and having one more that just says “Notice” might be confusing. Maybe let’s call it AlertNotice then? 😄

image

then that component should be renamed and moved from the SettingsNotice group to something not settings specific.

Since this issue had some refactoring already, I was skeptical of increasing the scope of this issue and test all existing SettingsNotice usage. I thought we could do the move refactor in #6702 as we will be testing all notices there for style changes throughout the plugin.

Sound good to me, thanks 👍

  • In assets/js/modules/adsense/components/dashboard, create and export a new functional component DashboardTopEarningPagesGA4NoticeWidget.

The widget should be named exactly as its UA counterpart but with GA4 suffix, the same as other widgets: DashboardTopEarningPagesWidgetGA4

@eugene-manuilov

We can use just one prop dismiss but make it be a string. If it’s empty, no need to render the dismissal button. If not empty, then render the button and use it as the dismissal item slug.

This is exactly what I started off with but then tried to keep things similar to BannerNotification. Also, passing the “dismissal key” slug into a dismiss prop felt less readable. But happy to reduce to one prop here if you felt the same.

then that component should be renamed and moved from the SettingsNotice group to something not settings specific.

Since this issue had some refactoring already, I was skeptical of increasing the scope of this issue and test all existing SettingsNotice usage. I thought we could do the move refactor in #6702 as we will be testing all notices there for style changes throughout the plugin.

I think instead of creating a new component for notice displaying purposes only, we need to create the GA4 copy of the widget and register the same way how we register other GA4 widgets to keep our approach consistent across all widgets.

Ah nice - didn’t realise we had that neat switcher! IB updated.