Open zutigrm opened 2 months ago
@zutigrm AC ✅ Moving to IB
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
.
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 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 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
- 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:
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 THanks, IB updated
Thanks, IB ✔
Feature Description
The
ACRSubtleNotification
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 panelFIgma 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
ConversionReportingSettingsSubtleNotification
component is updated to show/surface:Maybe later
CTA should dismiss the bannerSelect metrics
CTA should open the selection panelImplementation Brief
ConversionReportingNotificationCTAWidget
componentisUserInputCompleted
selector fromCORE_USER
store isfalse
isKeyMetricsSetupCompleted
istrue
getDetectedEvents
Analytics setting value is not an empty arrayMaybe later
CTA callback should behave in a same way as implementation in 9371Select metrics
CTA callback should open Selection panel https://github.com/google/site-kit-wp/blob/79b4417581b1cca3d718f7e55127681e6e05fc6f/assets/js/components/KeyMetrics/ChangeMetricsLink.js#L48ConversionReportingSettingsSubtleNotification
componentctaLabel
andctaCallback
ctaLabel
andctaCallback
to the primary button, and move existingAdd metrics
label and it's callback to theConversionReportingNotificationCTAWidget
to be used as a props forConversionReportingSettingsSubtleNotification
component following the initial conditions used for that banner variation.Test Coverage
ConversionReportingNotificationCTAWidget
(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 callbacksQA Brief
conversionReporting
feature flagSelect your own metrics
link in the key metrics setup CTA) and selecting some key metrics to display.Changelog entry