Key Metrics “Connect GA” CTA #7255
Conversation
|
Size Change: +1.52 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
Build files for 4952559 have been deleted. |
There was a problem hiding this comment.
Thanks @nfmohit – this is off to a great start. A few things to address and suggestions for you to consider.
Also, VRT is currently failing a few scenarios.
Addressed all of them, and updated VRT scenarios. Back to you for another pass, thank you @aaemnnosttv! |
There was a problem hiding this comment.
Thanks @nfmohit – I think we're almost there! Just a few things left for you.
tests/js/utils.js
Outdated
| export const provideWidgetRegistrations = ( | ||
| registry, | ||
| areaSlug, | ||
| areaTitle, | ||
| contextSlugs = null, | ||
| widgets | ||
| ) => { |
There was a problem hiding this comment.
This is a bit different than the convention shared by the other provide* utilities which all allow for easily providing something with sensible defaults that can be merged/overridden with customizations, but the utility itself should be useful without any (e.g. provideWidgetRegistrations( registry )).
This is a bit tricky for widgets since we don't support removing them (similar to modules). For a general widget utility like this, I'd imagine it would work very similar to the utility for module registrations (provideModuleRegistrations) which essentially iterates over all the core module JS-modules, and calls its registerModule export if it has one. This requires kind of decorating the module registration API call to allow for merging the extra data but the same should be possible here. We have the same kind of API for modules that have widgets as well via the registerWidgets export.
The difference with widgets is the hierarchical relationships between contexts, areas, and widgets which makes things a bit more complicated but still doable, however this is complicated and it seems out of scope to try and squeeze this much in at this point.
That said, let's move this function to be more of a local utility function with a note to refactor it in the future and open a new issue for the generic utility as described above. As a local helper, it's okay for the function to be more specific to what it's doing now so long as its clear what it's doing (particularly if it's shared across multiple files).
Finally, just a note about the function signature that was chosen here – we generally avoid functions with more than two positional arguments because it makes it difficult to reason about what each argument is when there are many. For this reason, you'll see the fn( a, args ) or fn( a, options ) as conventions we use quite a bit with the difference being whether or not the second argument is required or not.
There was a problem hiding this comment.
Thank you for the kind explanation and observation.
I absolutely agree. I have created #7264, and made the current utility specific to key metrics widgets with appropriate notes. Let me know if they look good, thanks!
Finally, just a note about the function signature that was chosen here – we generally avoid functions with more than two positional arguments because it makes it difficult to reason about what each argument is when there are many. For this reason, you'll see the
fn( a, args )orfn( a, options )as conventions we use quite a bit with the difference being whether or not the second argument is required or not.
Duly noted, understood, and agreed, thank you!
| if ( isFeatureEnabled( 'userInput' ) ) { | ||
| widgetsAPI.registerWidget( | ||
| 'keyMetricsConnectGA4CTA', | ||
| { | ||
| Component: ConnectGA4CTAWidget, | ||
| width: [ widgetsAPI.WIDGET_WIDTHS.FULL ], | ||
| priority: 1, | ||
| wrapWidget: false, | ||
| modules: [ 'analytics-4' ], |
There was a problem hiding this comment.
Let's move this widget registration to the GA4 module since it's essentially an activation CTA (all widgets are registered regardless of activation/connection). This module-specific widget shouldn't be registered from a core store. Alternatively, if GA was not available as a module at all, this CTA wouldn't be registered and shouldn't be shown.
There was a problem hiding this comment.
Good call! I've moved this inside the GA4 module. Thank you!
Commit: 5cee01e
assets/js/modules/analytics-4/components/widgets/ConnectGA4CTAWidget.js
Outdated
Show resolved
Hide resolved
Back to you for another pass, thank you @aaemnnosttv! |
Summary
Addresses issue:
Relevant technical choices
This PR implements the Key Metrics “Connect GA” CTA.
Difference from the IB
If you take a look at the Figma designs, it can be noticed that the Key Metrics Setup CTA widget and the Connect GA4 CTA widget have a lot in common when it comes to the layout.
As a result, instead of following the IB in this regard, I have used the layout elements implemented in the Key Metrics Setup CTA widget as a part of #6210 and extracted them into common layout components to re-use them in both CTA widgets.
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist