Skip to content
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

Implement “Connect AdSense” metric tile CTA after Key Metrics widget setup #6264

Closed
jimmymadon opened this issue Dec 2, 2022 · 8 comments
Labels
Exp: SP P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@jimmymadon
Copy link
Collaborator

jimmymadon commented Dec 2, 2022

Feature Description

If a selected metrics within the Key Metrics widget area depends on AdSense, but AdSense is disconnected (after previously having been connected), then a new CTA tile should be rendered to encourage the user to reconnect the module.

Refer to the relevant section in the design doc for additional context.

Screenshot 2022-12-14 at 12 49 16


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

Acceptance criteria

  • A new Connect AdSense CTA tile should be implemented as per the Figma mock.
  • The tile should be rendered if the "Top Earning Content" metric was selected (which is dependant on both, Analytics and AdSense) and AdSense was disconnected after the widget's setup.
  • Clicking on the link in the CTA should take the user to the Connect services page for AdSense.

Implementation Brief

  • Once Create ConnectGA4CTATileWidget key metrics widget #7153 PR has been merged, we will have access to ConnectModuleCTATile which makes this much simpler to implement.
  • In assets/js/googlesitekit/datastore/user/constants.js file:
    • At the moment, there is only one potential Key metrics widget that requires AdSense, however it is stalled.
    • a new constant KM_ANALYTICS_ADSENSE_TOP_EARNING_CONTENT should be defined.
    • Export a keyMetricsAdSenseWidgets array that contains all the Key Metrics Widget slugs that depend on AdSense module.
    • Also add the newly added constant to keyMetricsGA4Widgets array as this will also be dependent on GA4.
  • Create ConnectAdSenseCTATileWidget component in assets/js/modules/adsense/components/widgets/ConnectAdSenseCTATileWidget.js
    • It should be similar to ConnectGA4CTATileWidget.
    • It will receive Widget and WidgetNull as props.
    • Check whether adsense module is connected using isModuleConnected selector.
    • Get the keyMetrics widgets using the getKeyMetrics selector.
    • Define a hideWidget variable as initially false.
    • Assign hideWidget to false if AdSense module is not connected and the keyMetrics contains any tile that depends on adsense using the previously defined keyMetricsAdSenseWidgets array.
      • For now this can mirror the logic of Analytics.
    • Return WidgetNull if either AdSense module is connected or hideWidget is true.
    • Otherwise, render ConnectModuleCTATile component with the following props.
      • Icon: svg/graphics/adsense.svg (AdSenseIcon)
      • moduleSlug: adsense.
      • Widget: Widget
  • In assets/js/modules/adsense/index.js file:
    • Conditionally register the ConnectAdSenseCTATileWidget widget when userInput feature flag is enabled with the following config.
      • slug: keyMetricsConnectAdSenseCTATile
      • Component: ConnectAdSenseCTATileWidget
      • width: widgets.WIDGET_WIDTHS.QUARTER
      • priority: 1
      • wrapWidget: false
      • modules: ['adsense', ''analytics-4]
      • area: [ AREA_MAIN_DASHBOARD_KEY_METRICS_PRIMARY ]

Test Coverage

  • Add tests and storybook coverage for the ConnectAdSenseCTATileWidget component.

QA Brief

  • Ensure that the ga4Reporting and userInput feature flags are enabled.
  • Make sure AdSense module is connected.
  • Setup Key Metrics manually with the following snippet.
await googlesitekit.data.dispatch('core/user').setKeyMetricsSetting( 'widgetSlugs', ['kmAnalyticsAdSenseTopEarningContent', 'kmAnalyticsLoyalVisitors'] );
await googlesitekit.data.dispatch('core/user').saveKeyMetricsSettings();
  • Disconnect AdSense module.
  • See the new widget appear in the Key Metrics area.
  • Remove the Adsense dependent widget using the following snippet. ie.
await googlesitekit.data.dispatch('core/user').setKeyMetricsSetting( 'widgetSlugs', [ "kmAnalyticsPopularProducts"] );
await googlesitekit.data.dispatch('core/user').saveKeyMetricsSettings();
  • The widget should dispensary.

Changelog entry

  • Show a “Connect AdSense CTA” Key Metrics tile if AdSense is disconnected after setting up AdSense-related Key Metrics.
@jimmymadon jimmymadon self-assigned this Dec 2, 2022
@eclarke1 eclarke1 added P0 High priority Type: Enhancement Improvement of an existing feature labels Dec 2, 2022
@jimmymadon jimmymadon removed their assignment Dec 14, 2022
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Dec 14, 2022
@tofumatt tofumatt changed the title Implement “Connect AdSense” metric tile CTA after KM widget setup Implement “Connect AdSense” metric tile CTA after Key Metrics widget setup Dec 14, 2022
@tofumatt
Copy link
Collaborator

Moving to IB 👍🏻

@eclarke1 eclarke1 added P1 Medium priority and removed P0 High priority labels Dec 16, 2022
@sashadoes sashadoes self-assigned this Jan 4, 2023
@bethanylang
Copy link
Collaborator

@sashadoes Can you please unassign if you're not able to work on this so that someone else can pick it up? Thanks!

@sashadoes sashadoes removed their assignment Feb 7, 2023
@tofumatt tofumatt self-assigned this Feb 14, 2023
@tofumatt
Copy link
Collaborator

Render the link text within the Link component per the Figma design.

I know we do this elsewhere, and it's out-of-scope for this issue, but in the future we should ensure "Links" without a href are actually rendered in the DOM as buttons, not a. (This was highlighted in the accessibility review, see #173.)

Anyway, aside from that looks good. IB ✅

@tofumatt tofumatt removed their assignment Feb 14, 2023
@kuasha420 kuasha420 self-assigned this Jun 14, 2023
@kuasha420
Copy link
Collaborator

kuasha420 commented Jun 14, 2023

Pulling this one back to IB for a quick update after discussion here with @eugene-manuilov. Should be a quick one and make things simpler here!

cc @jimmymadon @bethanylang

@kuasha420 kuasha420 removed their assignment Jun 14, 2023
@tofumatt tofumatt self-assigned this Jun 15, 2023
@tofumatt
Copy link
Collaborator

Looks good to me, IB ✅

@tofumatt tofumatt removed their assignment Jun 15, 2023
@kuasha420 kuasha420 self-assigned this Jun 22, 2023
@kuasha420 kuasha420 removed their assignment Jun 22, 2023
@techanvil techanvil assigned techanvil and kuasha420 and unassigned techanvil Jun 23, 2023
@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Jun 23, 2023
@techanvil techanvil removed their assignment Jun 23, 2023
@wpdarren wpdarren self-assigned this Jun 23, 2023
@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@kuasha420 I have a few observations:

  1. When I pasted this code into the console await googlesitekit.data.dispatch('core/user').saveKeyMetricsSettings(); an error appeared which you can see in the screenshot below. When I disabled AdSense the tile did appear, but wanted to double check that this is not an issue.

  2. The placement of the tile within KMW is not the same as figma design. It's appearing as the 2nd tile rather than 4th. I feel this might be expected at this point of the engineering but wanted to check.

image

  1. Again, these issues could be expected: the size of the tile is bigger than the figma designs. 375x133.47 is what I see on my test site. On figma the dimensions is 265 x 142. Also the CTA link font colour is #3c7251 but on Figma it is #108080 Finally, the AdSense logo is different on my site compared with figma. It's 35 x 35 on my site but 35x30 on Figma.

image

Interested in your thoughts.

@kuasha420
Copy link
Collaborator

@wpdarren Thank you for sharing your observation.

  1. This is unrelated to the issue and we don't have to be concerned with it at the moment.
  2. This is also expected at this stage.
  3. Sizes are responsive and can be different than figma in terms of overall dimension. As long as the margin, borders etc are fine, I think this is not an issue. The CTA link color is a real issue and the follow up PR for Implement “Connect GA” metric tile CTA after Key Metrics widget setup #6263 will also fix this here. Logo size is also consistent with other areas in plugin where they're assumed to be square.

Thank you.

@kuasha420 kuasha420 removed their assignment Jun 26, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • The new widget appears in the Key Metrics area when AdSense is disabled.
  • The Adsense widget disappears when the code is run in the browser console.
  • Checked that the tile looks as per the figma designs

image

@wpdarren wpdarren removed their assignment Jun 26, 2023
@bethanylang bethanylang added P0 High priority and removed P1 Medium priority labels Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants