Enhance/8156 connect analytics cta #8798
Conversation
|
Build files for e03170d have been deleted. |
There was a problem hiding this comment.
Excellent work, @kuasha420! The changes look good overall. However, please consider addressing the following issues:
- Tablet Breakpoint: The tablet breakpoint should start from
601px, consistent with other CTAs likeConnectModuleCTATile. Notably, the period before theConnect Google Analyticsbutton was missing. We calculate the tablet breakpoint from601px. See:
site-kit-wp/assets/js/hooks/useBreakpoint.js
Lines 47 to 49 in 27d72f1
Please refer to the screencast below for details:
Screen.Recording.2024-06-07.at.3.51.45.PM.mov
- CTA Text Alignment: The CTA text is not aligned properly at
960px. This should be corrected.
- Desktop Breakpoint: We should use
961px($width-desktop + 1px) for the desktop breakpoint instead of960px.
- Minor Comments: I've left a few nit-picky comments. Please take a look at them.
Thanks!
| @@ -44,6 +44,7 @@ import KeyMetricsNewBadge from '../../components/KeyMetrics/KeyMetricsNewBadge'; | |||
| import ConnectGA4CTAWidget from '../../modules/analytics-4/components/widgets/ConnectGA4CTAWidget'; | |||
| import { AudienceAreaFooter } from '../../modules/analytics-4/components/audience-segmentation/dashboard'; | |||
| import { isFeatureEnabled } from '../../features'; | |||
| import ConnectAnalyticsCTATileWidget from '../../modules/analytics-4/components/audience-segmentation/dashboard/ConnectAnalyticsCTATileWidget'; | |||
There was a problem hiding this comment.
A nit-picky one, let's add this import above along with other component imports.
|
|
||
| const { useSelect } = Data; | ||
|
|
||
| export default function ConnectAnalyticsCTATileWidget( { Widget } ) { |
There was a problem hiding this comment.
Can we add PropTypes for the Widget prop?
| provideModules( registry, [ | ||
| { | ||
| slug: 'analytics-4', | ||
| active: false, | ||
| connected: false, | ||
| }, | ||
| ] ); |
There was a problem hiding this comment.
This seems unnecessary. It works without provideModules. We only need to provideModuleRegistrations for the Analytics icon to be rendered.
| isActive: ( select ) => { | ||
| const isAnalyticsConnected = | ||
| select( CORE_MODULES ).isModuleConnected( | ||
| 'analytics-4' | ||
| ); | ||
|
|
||
| return ! isAnalyticsConnected; | ||
| }, |
There was a problem hiding this comment.
IMO, the one-liner seems cleaner. It's just a personal preference, and applying it is unnecessary unless you are okay with it :)
| isActive: ( select ) => { | |
| const isAnalyticsConnected = | |
| select( CORE_MODULES ).isModuleConnected( | |
| 'analytics-4' | |
| ); | |
| return ! isAnalyticsConnected; | |
| }, | |
| isActive: ( select ) => | |
| ! select( CORE_MODULES ).isModuleConnected( 'analytics-4' ), |
There was a problem hiding this comment.
This will change in a follow up issue where it will also take the audience setup into consideration, when it will no longer work as a simple one liner, I've added a TODO comment to explain this. Cheers.
|
Size Change: +12.6 kB (+0.82%) Total Size: 1.55 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks, @kuasha420. Could you please fix the viewport range to be from 961px to 1139px?
Update: Can we push the SVG to the right for the above viewport?
|
@hussain-t Thank you for the review and pointer. This is looking much better now but needed some tinkering with the SVG and flex properties, let me know what you think! Cheers. |
There was a problem hiding this comment.
Thanks for fixing the issues, @kuasha420. It looks a lot better. However, I am concerned that on larger screens, there's excessive whitespace on the right. Can we adjust the graphics container to use justify-content: flex-end for screens that are 1440px and wider?
@media (min-width: $bp-xxlarge) {
justify-content: flex-end;
margin-right: 50px; // Adds a bit more spacing on the right
}Additionally, please resolve the merge conflicts.
Update: Since we don't have designs for large/wide screens, we can leave it as it is.
There was a problem hiding this comment.
Excellent work here, thank you @kuasha420! I have left a few questions for your consideration. Let me know what you think, thanks!
...ules/analytics-4/components/audience-segmentation/dashboard/ConnectAnalyticsCTATileWidget.js
Outdated
Show resolved
Hide resolved
...lytics-4/components/audience-segmentation/dashboard/ConnectAnalyticsCTATileWidget.stories.js
Outdated
Show resolved
Hide resolved
Summary
Addresses issue:
Relevant technical choices
Deviation from IB: Currently in this PR, the Widget will be shown unconditionally when Analytics module is disconnected. This is because at the moment, audience settings are part of Analytics module, so we can't check it when the module is disconnected. This will be fixed in #8810. The AC and QAB has been updated to reflect that, so there should be no further confusion in QA.
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