-
Notifications
You must be signed in to change notification settings - Fork 293
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
Update GA4 Disconnected Banner CTA design when 3 or 4 metric tiles rely on GA4 #7278
Comments
Need to wait for the blocking issues here to be merged before we finalise the AC here. c.c. @bethanylang |
Hi, @jimmymadon! Just a heads-up that I code-reviewed #7061 and it does look like we'll need to proceed with this issue. Currently, we're rendering a metric widget based on its availability in the response of the I don't think it needs to, but at least we should update the widget registrations for GA4-dependent metrics only to be active when GA4 is connected, I think that will most likely solve the issue. Thanks! |
@nfmohit Thanks! I think the |
@jimmymadon should any requests be made at all from non-rendered widget tiles? I would think that really no side-effects should be triggered in this case since the widgets wouldn't be rendered at all? WDYT? |
@aaemnnosttv Absolutely - no requests should be made in the first place. I have worded this better. |
Thanks @jimmymadon 👍 AC ✅ |
Reducing the priority here as discussed as non-critical for launch and more thought is needed around the full-width widget special cases. |
As the description suggests, when engineering the full-width "Connect GA4 CTA Banner" @aaemnnosttv realised that the approach to render this banner was not in line with our existing Widgets API infrastructure. In order to reuse the Widgets API, we have decided to "stretch" the single "Connect GA4 CTA Tile" to cover as many spaces as there were GA4 widget tiles. So we are deviating slightly from the design doc here. Originally, if 1 or 2 metric tiles were reliant on GA4, we would show the individual "Connect GA4 CTA" tile. If 3 or 4 metric tiles were reliant on GA4, then we would show the banner. Now, in 7337, we have decided to stretch the individual "Connect GA4 CTA" tile to take up either 1, 2, 3 or 4 spaces depending on how many ever GA4 tiles there were. The main deviation here is when 3 metrics are GA4 tiles and 1 metric is a Search Console tile. Originally, we planned to not show the SC tile and simply render the "Connect GA4 CTA" full width banner. This "special case", while possible, is causing us to deviate from patterns we use in our existing widgets API infrastructure. We also think that showing the single SC metric tile data might be helpful to some users who temporarily have some problems connecting GA4 (and not force them to hide the Key Metrics widget completely). This issue is being treated as a "Nice to have" currently and will be implemented in the bug bash remediation sprints and so we would like to (re)define this. So in the event where 1 of the metric tiles is dependant on SC and the remaining three are dependant on GA4, how should we proceed? Some potential options:
|
Enhance/#7278 - Update GA4 Disconnected Banner CTA
QA update ❌Issue 1: I found little awkward responsive issue when viewport size is 601px to 960px. Can we show CTA with 1 ghost tile or no ghost tile in this case ? When 2 KMW Tiles Reliant on GA4: When 3 KMW Tiles Reliant on GA4: Issue 2 : Different wording on CTA version For 4 KMW Tiles Reliant on GA4 and For 3 KMW Tiles Reliant on GA4 or For 2 KMW Tiles Reliant on GA4 Issue 3 : 'May be later' button is not functional. Nothing happen when user clicks on 'May be later' button. |
Thanks, @mohitwp. Regarding your observations:
The tile is displayed in full width regardless of the ghost cards. It's an existing issue. You can verify it in the I want to clarify the visibility of the ghost cards again, it is displayed based on the container width, i.e., the size of the CTA banner width, not the viewport width. Please refer to this point in the IB:
CTA without the Ghost Cards
This is not related to this issue. @jimmymadon can provide if keeping the text as it is or changing would be preferred.
This is a valid issue. I will fix it in a follow-up PR. |
Enhance/#7278 - Dismiss GA4 tile banner.
Back to you for another pass, @mohitwp. |
I would like to get @sigal-teller's / @marrrmarrr's opinion here. Currently, we use the "module's name" for this CTA since it is being used by other modules as well (AdSense). When referring to Analytics throughout the plugin, we always just use "Analytics". Should the "Disconnected Module CTA"'s here be an exception where we use "Google Analytics" or should we stick to just "Analytics"? Or should we consider changing the name of Analytics to Google Analytics all throughout the plugin? |
@jimmymadon Site Kit is Google's official plugin, so all the services connected are by default Google services. So I don't think it's necessary to explicitly append "Google Analytics" everywhere. |
QA Update ✅
|
Feature Description
When GA4 is disconnected and when more than 2 of the KMW tiles are dependant on GA4, we show the "Connect GA4 CTA Banner" in #6265. However, this behaviour was changed in #7337 where the individual "Connect GA4 CTA Tile" would stretch to take the space of as many GA4 widget tiles. This means that if 3 of the tiles are associated with GA4, a longer CTA tile will be displayed. The individual CTA was originally designed to cover one tile - now that this is getting longer and could potentially cover all 4 spaces, it should somehow "switch" to using the "Connect GA4 CTA Banner" component that was created in #6265.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Maintain Existing Display for 1 or 2 Metric Tiles:
Demonstrative POC Branch:
Implement Extended "Connect GA4 CTA Tile" (For 3 Tiles):
In
assets/js/components/KeyMetrics/ConnectModuleCTATile.js
:GhostCardGreenSVG
andGhostCardRedSVG
components inside adiv
.Grid->Row->Cell
structure to wrap elements.Register the "Connect GA4 CTA" widget (for 4 tiles)
In
assets/js/googlesitekit/widgets/register-defaults.js
:ConnectGA4CTAWidget
widget with the following slug and options:keyMetricsConnectGA4CTA
or any appropriate slug.Component
should beConnectGA4CTAWidget
.width
should be full width.priority
should be1
.wrapWidget
should befalse
.isActive
should check if GA4 is disconnected and if 4 of the selected metric tiles rely on GA4.AREA_MAIN_DASHBOARD_KEY_METRICS_PRIMARY
.In
assets/js/modules/analytics-4/components/widgets/ConnectGA4CTATileWidget.js
:Null
component as the second argument in theuseWidgetStateEffect
hook. Else, use theConnectModuleCTATile
component.In
assets/js/modules/analytics-4/components/widgets/ConnectGA4CTAWidget.js
:WidgetNull
to check if 4 of the selected metric tiles rely on GA4.Update the Stories
In
assets/js/modules/analytics-4/components/widgets/ConnectGA4CTATileWidget.stories.js
:Test Coverage
QA Brief
Adding a note here for QAB from #7285: When we test this ticket, we also need to go through these steps:
Update
Changelog entry
The text was updated successfully, but these errors were encountered: