-
Notifications
You must be signed in to change notification settings - Fork 284
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
Create the Acknowledgement Component #5279
Comments
Should we be using "GA4" as a shorthand for this? I don't think we actually refer to Google Analytics 4 as "GA4" anywhere user-facing in the plugin. I've changed the user-facing text in the ACs from "GA4" to "Google Analytics 4". If that's no good just let me know @techanvil 🙂 |
Hey @tofumatt - turns out, we do refer to it as "GA4" in a number of user-facing spots. I have found references in: https://github.com/google/site-kit-wp/blob/develop/assets/js/modules/analytics/components/common/GA4Notice.js And, also in the support page we link to: https://sitekit.withgoogle.com/documentation/using-site-kit/ga4/ Sooo... I mean, I don't see anything actually wrong with the change you've made, but we also certainly do use the abbreviation in a user facing way. |
Hey @jimmymadon, you have raised an interesting point in the IB about code reuse. On reflection, I actually think this is a case where we should go a bit further. The code to interact with the WordPress Admin Menu is quite fiddly and I think it's something that would really benefit from not duplicating at all. Furthermore the functionality of showing a tooltip pointing to Settings (or thinking more generally, the Admin Menu), now that it's being used in two places, it feels like there's a good chance that we'll want to use it more as time goes on. With that in mind, I would suggest that we take the approach of extracting the related code from I've knocked up a quick draft PR, which could be implemented pretty much as-is or serve as a basis for doing so: #5654 If you are happy taking the approach in the PR, I think it would be fair to reference it in the IB without going into all the steps in detail, rather a point about extracting the behaviour and link to the PR should suffice, along with a point(s) about using it in the What do you think? |
@techanvil Great refactor - thank you! Update the IB. |
Thanks @jimmymadon! IB ✅ |
@jimmymadon just a heads up that I added a point to the QAB about regression testing the "Maybe later" behaviour of the AdSense Connect CTA due to the refactor. |
QA Update: ✅Verified:
|
Feature Description
Create the Acknowledgement Component. This will take the form of a tooltip, with the following text:
Title: You can connect Google Analytics 4 later here
Description: You can configure the Google Analytics 4 property inside the Site Kit Settings later.
This component will take a prop for the button text.
Display the Acknowledgement Component when postponing the GA4 Activation Banner:
From the Design Doc:
Acceptance criteria
Implementation Brief
assets/js/modules/adsense/components/dashboard/AdSenseConnectCTAWidget.js
so use it as a reference for each of the steps below. The reusable logic to display anAdminMenuTooltip
has been extracted into a separate reusable component this draft PR to avoid duplicating the logic for this issue. Use this POC as a starting point and refer to how it is being used withinAdSenseConnectCTAWidget
.assets/js/modules/analytics-4/constants.js
forACTIVATION_ACKNOWLEDGEMENT_TOOLTIP_STATE_KEY
that will be used as the key to persist the state of theAdminMenuTooltip
(show/hide along with admin menu state).assets/js/modules/analytics-4/components/dashboard/ActivationBanner/ReminderBanner.js
andSetupBanner.js
:AdminMenuTooltip
component with thedismissLabel
prop set to "Got it" (forReminderBanner
) or "Remind me later" (forSetupBanner
) as in the AC.Test Coverage
QA Brief
Changelog entry
The text was updated successfully, but these errors were encountered: