-
Notifications
You must be signed in to change notification settings - Fork 278
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 GA” CTA after Key Metrics widget setup with GA4 disconnected/unavailable #6265
Comments
From the Figma mocks, it looks like the CTA widget could be rendered as a half-width widget if it makes sense (eg. with multiple widgets specified, like
But that can be addressed in the IB. Anyway, ACs here look good, moving to IB 👍🏻 |
All looks good, just one minor naming point; the point of the module is to Connect GA4, so might as well name it IB ✅ |
I'm seeing a discrepancy between the ACs and the IB here regarding the The ACs say:
The IB says:
I do not think connecting the module would be as simple as the SetupModule component because there are aspects of selecting an Analytics account, property, web data stream, and error handling. @jimmymadon Any advice on which approach should be taken here? CC: @hussain-t @tofumatt |
@nfmohit I have updated the ACs here. I think we can use the same behaviour as in the "Set up Google Analytics" CTA within the Search Traffic widget. Would this be fine? |
That's perfect, thank you @jimmymadon! |
QA Update:
|
Thank you for sharing your observations, @wpdarren! @jimmymadon Could you possibly help a little to answer Darren's questions here?
The ACs of this issue don't mention about updating the visibility of the other widgets. Is this something that will be addressed by another issue?
According to my observation, these errors are caused by the other widgets making GA4 report requests. Not rendering them when GA4 is disconnected should solve it.
This is indeed a valid concern. What do you think of setting an expiry to the dismissal so that it expires after a while, say 24 hours? That way, it'll not show up if GA4 is connected again, but will show up again if it is disconnected again. Thank you! |
#7061 will prevent other widget tiles from showing up as long as the
Yup - I think so too.
I feel we should "reset" this persistent dismissal when GA4 is "connected" rather than relying on expiry times. We could perhaps do this in the |
That is a good idea. However, if I'm not wrong, I do not think we have in our infrastructure a way to remove a dismissed item. One way could be updating the dismissed item with a very small expiry, e.g. 1 second. Otherwise, we'll have to enhance the Additionally, I think it'd be wiser to address this in a new issue as everything else is according to spec. @jimmymadon What do you think? Thank you! @eugene-manuilov @aaemnnosttv @tofumatt @techanvil Requesting feedback from one/more of you here too please, thank you! |
It seems reasonable that we should allow for removing a dismissed item. We could technically do it now as you said by re-adding it again with the smallest possible TTL, but that would be unnecessarily indirect to avoid adding a simple method to do the same thing explicitly.
Agreed, this is outside of the scope and also involves some changes that wouldn't be entirely behind the feature flag so it's best to handle this separately. |
QA Update: ✅Verified:
Please note that 3 issues were identified above. All will be fixed in other tickets. |
Feature Description
If more than two of the selected metrics within the Key Metrics widget area are from GA4, but GA4 is disconnected, then a new CTA banner should be rendered to encourage the user to reconnect the GA.
Refer to the relevant section in the design doc for additional context.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Note that the width of the tile should be the same as a widget's standard "quarter" width and not 1/3rd as in the Figma design.Implementation Brief
Create the components
ConnectGA4CTA
component in the same directory Scaffold Key metrics Setup CTA on Main Dashboard #6209 is implemented.section
tag. Followed byGrid->Row->Cell
components.h3
tag per the Figma design. The text should be translated.p
tag per the Figma design. The text should be translated.Connect Google Analytics
button within theButton
component per the Figma design. The text should be translated. TheonClick
callback should follow a similar implementation as theSetupModule
component:site-kit-wp/assets/js/components/settings/SetupModule.js
Lines 57 to 82 in 43c4364
footer
tag. Followed byGrid->Row->Cell
components.Maybe later
link within theLink
component per the Figma design. The text should be translated.onClick
callback should dismiss the CTA by dispatching thedismissItem
action. An appropriate constant key should be created to pass it to the action.null
if the GA4 module is connected using theisModuleConnected
selector.getKeyMetrics
selector, the component should check if any of the selected metrics are from GA4. If so, it should render theCTA
component. Otherwise, it should rendernull
.ConnectGA4CTAWidget
component in the same directory Scaffold Key metrics Setup CTA on Main Dashboard #6209 is implemented.Widget
andWidgetNull
components as props.ConnectGA4CTA
component within theWidget
component if the following conditions are met:isItemDismissed
selector.isModuleConnected
selector.getKeyMetrics
selector.WidgetNull
component.ConnectGA4CTAWidget
component.Register the widget
ConnectGA4CTAWidget
component in theKeyMetrics
widget area once Register the new key metric widgets #6313 is merged.width
should beFULL
.wrapWidget
should befalse
.priority
should be1
.modules
should be['analytics']
.Test Coverage
ConnectGA4CTA
component.QA Brief
userInput
feature flag.Publish a blog
for the first (purpose) question. This should configure key metrics to display widgets all dependant on Analytics.Google Analytics is disconnected
CTA widget, according to the ACs and Figma designs.Maybe later
should dismiss the widget.Key Metrics -> ConnectGA4CTAWidget
Storybook story.Changelog entry
The text was updated successfully, but these errors were encountered: