site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Expand the logic of the `ACRNotificationCTAWidget` component for existing KMW users with manual set up

Open zutigrm opened this issue 1 year ago • 8 comments

Feature Description

The ACRSubtleNotifciation component logic should be expanded so it surfaces in the dashboard within key metrics widget area for the users who have setup KMW manually, once ACR events are detected. CTA should open selection panel

FIgma design can be found here

Refer to the Subtle banner notification for detected events section of the design doc


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

Acceptance criteria

  • Following the implementation in 9371 ACRDashboardSubtleNotification component is updated to show/surface:
    • If key metrics were setup using manual selection
    • And initial set of conversion events have been detected
  • Said component should match the figma design
  • Clicking the Maybe later CTA should dismiss the banner
  • Clicking the Select metrics CTA should open the selection panel
  • Check the Subtle banner notification for detected events section of the design doc

Implementation Brief

  • [ ] Update ACRNotificationCTAWidget component
    • Banner should render if:
      • isUserInputCompleted selector from CORE_USER store is false
      • And isKeyMetricsSetupCompleted is true
      • And getDetectedEvents Analytics setting value is not an empty array
      • Following above conditions, include the CTA variation/callback:
        • Maybe later CTA callback should behave in a same way as implementation in 9371
        • Select metrics CTA callback should open Selection panel https://github.com/google/site-kit-wp/blob/79b4417581b1cca3d718f7e55127681e6e05fc6f/assets/js/components/KeyMetrics/ChangeMetricsLink.js#L48
  • [ ] In In ACRDashboardSubtleNotification component
    • Include two props: ctaLabel and ctaCallback
    • Apply ctaLabel and ctaCallback to the primary button, and move existing Add metrics label and it's callback to the ACRNotificationCTAWidget to be used as a props for ACRDashboardSubtleNotification component following the initial conditions used for that banner variation.

Test Coverage

  • Update tests for ACRNotificationCTAWidget (basic coverage added in 9344), to verify that banner is showing when conditions are met, and not showing otherwise for example. Also tests for new CTA's variation and their callbacks

QA Brief

Changelog entry

zutigrm avatar Sep 19 '24 11:09 zutigrm

@zutigrm AC ✅ Moving to IB

10upsimon avatar Oct 01 '24 05:10 10upsimon

Thanks, @zutigrm, mostly looks good to me.

  • Ensure that conditions apply only when component is shown outside of VIEW_CONTEXT_SETTINGS context

Which conditions? The ones from the next point? Plus instead of using negative statement we should explicitly say for which context it should be displayed. In other words, instead of component is shown outside of ... context, we should say component is shown in A, B, C, D context.

eugene-manuilov avatar Oct 05 '24 14:10 eugene-manuilov

Thanks @eugene-manuilov I updated IB to list the contexts

Which conditions? The ones from the next point?

I expanded on this sentence to clarify that it should apply for conditions in next points

zutigrm avatar Oct 10 '24 09:10 zutigrm

@zutigrm ok, since we updated #9371, we need to update IB for this ticket as well. Instead of adding conditions to the notification component, we need to update the widget to render different notifications based on aforementioned conditions. I think we can probably re-use the notification component from #9371 because it has similar content, right?

eugene-manuilov avatar Oct 15 '24 13:10 eugene-manuilov

@eugene-manuilov Thanks, updated.

I think we can probably re-use the notification component from https://github.com/google/site-kit-wp/issues/9371 because it has similar content, right?

Yes that's the idea, that component will be reused and just conditions would be expanded

zutigrm avatar Oct 16 '24 11:10 zutigrm

  • Following above conditions, include the content variation matching this banner in Figma

@zutigrm the only difference between two versions of the notification is the CTA button:

Image Image

So, there is no need to add any additional logic to it. Instead we need to add two props to the notification to set CTA label and callback. And then in the widget, we need to use different CTA labels and callbacks based on the defined criteria.

eugene-manuilov avatar Oct 18 '24 16:10 eugene-manuilov

@eugene-manuilov THanks, IB updated

zutigrm avatar Oct 21 '24 07:10 zutigrm

Thanks, IB ✔

eugene-manuilov avatar Oct 21 '24 09:10 eugene-manuilov

QA Update ⚠

Hi @benbowler , referring to the QAB where it mentions about setting up the Tweak extension. I am not 100% clear on its usage and when is it being triggered. Can you clarify on that? Also, what are the data to be filled for the response payload, request payload and status code?

Image

kelvinballoo avatar Dec 08 '24 13:12 kelvinballoo

@kelvinballoo To chip in here, since you have the tester plugin I sent you, you won't need to use tweak, surfacing the callout by forcing the events will suffice, since once you click on it it will be dismissed without usage of extensions

zutigrm avatar Dec 09 '24 08:12 zutigrm

QA Update ⚠

ITEM 1: ⚠ When clicking on 'Maybe Later' (Dismiss), the banner will disappear as expected. However, after reloading the page, the banner reappears. Is it because of the tester plugin functionality? Or we need to build a logic that after it's dismissed, it won't re-appear again?

Similarly when we click on 'Select metrics', the banner will disappear. However, after reloading the page, it will re-appear.

Unless these will be tackled under a separate ticket?

Dismiss button:

https://github.com/user-attachments/assets/b569a6f8-1665-4e86-a56e-c92d52608fdb

Select metrics scenario:

https://github.com/user-attachments/assets/efeb640e-7913-48af-a3f2-d960310372fa


ITEM 2: ⚠ The margin above the banner is supposed to be 12px based on Figma. In our implementation, it's 24px.

Figma:

Image

Implementation:

Image


ITEM 3: ⚠ Let's say we have the following scenario:

  • Some metrics are not selected. The banner will appear.
  • All the metrics related to the particular event are now checked.
  • The banner disappears accordingly.
  • The thing now is that if I uncheck one of the metrics, the banner will resurface again. I assume that's not expected. Are we handling this in a separate ticket?

https://github.com/user-attachments/assets/3be67366-facc-428b-a931-c0bf9664c148


Tested the following and they were as expected: ✅

  • Scenario 1 - Select metrics banner appears accordingly

    https://github.com/user-attachments/assets/3b478e5a-10c6-4f34-a67c-89413c3d280e

  • When clicking on Select metrics, the panel selection will appear accordingly.

    https://github.com/user-attachments/assets/a66e8945-77a7-4ecf-a664-fb0343838a31

  • Scenario 2 - The banner appears again when not all the metrics were selected.

    https://github.com/user-attachments/assets/3c6fc5f3-7b25-4573-9bcc-0fb95c612186

  • Scenario 3 - If all the relevant metrics are selected, the banner will not appear.

    https://github.com/user-attachments/assets/7c2c99e8-c516-4b8f-ae7b-1cc4769c8550

  • Tested other scenarios pertaining to purchases and leads (contact, generate lead, submit lead form). Behaviours are similar as above.

    https://github.com/user-attachments/assets/1c2ff58a-0eaf-429d-bf48-baacb4efcebe

    https://github.com/user-attachments/assets/ce457962-568b-4390-b9ac-1362a49a2830

kelvinballoo avatar Dec 10 '24 12:12 kelvinballoo

@kelvinballoo Thanks for you observations. I can answer items 1 and 3:

ITEM 1: ⚠ When clicking on 'Maybe Later' (Dismiss), the banner will disappear as expected. However, after reloading the page, the banner reappears. Is it because of the tester plugin functionality? Or we need to build a logic that after it's dismissed, it won't re-appear again?

Similarly when we click on 'Select metrics', the banner will disappear. However, after reloading the page, it will re-appear.

Unless these will be tackled under a separate ticket?

Yes, this is due to the tester plugin, as long as you have forced new events, it will override the transient, and banner will re-appear on page load, this is expected. To simulate the dismissal, in real usage, after you dismiss the banner, disable override of the new events - as dismissal removed that transient. In this case on reload you will not see the banner any more

ITEM 3: ⚠

Let's say we have the following scenario:

Some metrics are not selected. The banner will appear. All the metrics related to the particular event are now checked. The banner disappears accordingly. The thing now is that if I uncheck one of the metrics, the banner will resurface again. I assume that's not expected. Are we handling this in a separate ticket?

Interesting observation. Basically banner will be visible until user clicks on one of the CTAs, select metrics will dismiss the banner, as well as maybe later. Once dismissed, transient will be deleted, so if user adds new metrics through select metrics CTA, and then de-selects them banner will not re-appear, if banner was not actioned and users chooses new metrics, saves them and then de-select one, banner would re-appear. As it is there to notify user about new events until dismissed. In this far edge case, the user would just need to dismiss it. Not sure if should consider additional scenario, WDYT?

zutigrm avatar Dec 10 '24 12:12 zutigrm

@kelvinballoo thinking more about item 3, it is better if we dismiss the banner, since even after several days, or week + if user deselect the item, banner will re-appear if not dismissed, which would be a bit confusing. We will have new badges appearing next to the metrics, but still yeah, let's dismiss this.

@benbowler We can update https://github.com/google/site-kit-wp/blob/548bf6b5e11b200b4928fe0eeab5ed4f35c478e2/assets/js/components/KeyMetrics/MetricsSelectionPanel/Footer.js#L144 callback to run same check we use for banner to appear shouldShowCalloutForUserPickedMetrics, and if it is false and there are conversionReportingEventsChange?.newEvents, invoke the dismissal action

If it will take more time we can also make it as a follow up issue

zutigrm avatar Dec 10 '24 13:12 zutigrm

@kelvinballoo After checking the item 2, we have unified margin on all widget area headers (which is a global element) which is 24px. For consistency doesn't seem like something we should change, in the Figma, spacing differs only for this scenario of notification showing, and by default without notification it seems to be mistakenly reduced in Figma. We can omit this one from changing.

I have opened a follow up issue (#9870) for item 3 since it is not scoped as part of this issue which only works on adding the callout variation, and so we can properly define this new change.

zutigrm avatar Dec 11 '24 13:12 zutigrm

QA Update ✅

Thanks @zutigrm

ITEM 1 is tested good after doing the steps to remove the detection of the events.

https://github.com/user-attachments/assets/9c358c17-9368-4717-9e29-b31956c4b51e

Noted on ITEM 2 and 3.


Tested the following and they were as expected as well: ✅

  • Scenario 1 - Select metrics banner appears accordingly

    https://github.com/user-attachments/assets/3b478e5a-10c6-4f34-a67c-89413c3d280e

  • When clicking on Select metrics, the panel selection will appear accordingly.

    https://github.com/user-attachments/assets/a66e8945-77a7-4ecf-a664-fb0343838a31

  • Scenario 2 - The banner appears again when not all the metrics were selected.

    https://github.com/user-attachments/assets/3c6fc5f3-7b25-4573-9bcc-0fb95c612186

  • Scenario 3 - If all the relevant metrics are selected, the banner will not appear.

    https://github.com/user-attachments/assets/7c2c99e8-c516-4b8f-ae7b-1cc4769c8550

  • Tested other scenarios pertaining to purchases and leads (contact, generate lead, submit lead form). Behaviours are similar as above.

    https://github.com/user-attachments/assets/1c2ff58a-0eaf-429d-bf48-baacb4efcebe

    https://github.com/user-attachments/assets/ce457962-568b-4390-b9ac-1362a49a2830

kelvinballoo avatar Dec 12 '24 08:12 kelvinballoo