Only show one of the Consent Mode, Ads, and Audience Segmentation Setup CTA Banners at a time. about site-kit-wp HOT 4 OPEN

@ankitrox could you please add the estimate to this ticket? That should be included when submitting to IBR, so the reviewer can review the estimate as well. Thank you!

from site-kit-wp.

Added the estimate in the ticket.

from site-kit-wp.

Thank you for the excellent IB, @ankitrox ! The approach looks good. I have some minimal feedback:

• Create a component SetupCTAWidget inside assets/js/components/setup/

I'd recommend that the component stays in assets/js/components. assets/js/components/setup mostly includes components related to Site Kit and module setup, which I think are slightly different than our context here.

• As most of the elements for rendering across AudienceSegmentationSetupCTAWidget and ConsentModeSetupCTAWidget is same, extract the SetupCTAWidget component which will render the common markup.

It might be worth mentioning that the extracted SetupCTAWidget component should use the re-usable generic setup banner classes introduced in #8660 (commit), i.e. .googlesitekit-setup-cta-banner* (instead of .googlesitekit-widget-audience-segmentation* for example).

• It's worth noting that while the banners all have a very similar look and feel, the Audience Segmentation one does use smaller font sizes for its title and description, which is something we should maintain (at least for now). Reusing the generic setup banner classes is the way to go, most likely we just need a couple of minor overrides for the AS banner which can be done using an additional custom class name for the Audience Segmentation banner.

• SetupCTAWidget should accept the prop like title, description, handleCTAClick, handleDismiss, SVGItemComponent and setupSlug.

  • title would be the title for the setup CTA widget.
  • description would be the description for the widget.
  • handleCTAClick callback when the CTA is clicked.
  • handleDismiss callback when the banner is dismissed.
  • SVGItemComponent component to render the SVG element. It can be wrapped up inside Cell component as both ConsentModeSetupCTAWidget and AudienceSegmentationSetupCTAWidget renders it within Cell.
  • setupSlug can be audience-segmentation or consent-mode. This is useful to keep the css classes intact. We can use this prop where classes like googlesitekit-audience-segmentation-setup-cta-widget or googlesitekit-consent-mode-setup-cta-widget are being used, so that we can insert like googlesitekit-${setupSlug}-setup-cta-widget.

• I think we'd also want to include Widget & WidgetNull components, as our extracted SetupCTAWidget component should also include the widget context and area structures.
• Some setup CTA banners, like AudienceSegmentationSetupCTAWidget & AdsModuleSetupCTAWidget include distinct graphics for tablet and mobile breakpoints. I'd recommend looking at how the OverlayNotification handles this and introducing GraphicDesktop, GraphicTablet, and GraphicMobile props. Of course, all of them would be optional, with each graphic rendered for its respective breakpoint, and GraphicDesktop rendered by default if the others are not defined.
• If the purpose of the setupSlug prop is to populate the class name, I'd recommend just introducing a className prop instead, like we do for most other components.

• Extracted component can manage some local states inside it like isSaving and saveError.

I'd recommend against letting the extracted SetupCTAWidget component handle isSaving and saveError as local states. As the save function is defined by its consumer, the loading state and error should be done similarly as well. isSaving and SaveError can be accepted as props for the SetupCTAWidget component and used accordingly.

• AudienceSegmentationSetupCTAWidget and ConsentModeSetupCTAWidget components can reuse the extracted component and pass the relevant props to it.

I understand this is not a part of the ACs, but I think it might be worth also including AdsModuleSetupCTAWidget in this batch as that is based on the same fundamentals. I presume it should be as simple as just letting it use our SetupCTAWidget component.

• Add a selector displayConsentModeSetupWidget in consent-mode data store. Condition to display the widget can be found in ConsentModeSetupCTAWidget component here.
• In DashboardMainApp, check if the consent mode setup widget can be display using the selector. If it can be displayed, do not render AudienceSegmentationSetupCTAWidget, else it should be rendered.

I understand that this is how the AC specs it out, however, looking at the bigger picture, we currently have (e.g. AdsModuleSetupCTAWidget) and in the future will have (e.g. setup CTA banner for the upcoming RRM integration) more similar setup CTA banners. Even though this spec meets the AC by showing either the AudienceSegmentationSetupCTAWidget component or ConsentModeSetupCTAWidget, this solution will not solve the forthcoming similar multiple widget scenario and prioritisation issues.

Keeping the above in mind, I'd recommend introducing a new assets/js/components/SetupCTAWidgets.js component. This component can include an array of setup CTA banner components to show, in the order of priority. The component can then iterate through each of the items in the array, and render the first one that does not return null.

The SetupCTAWidgets component can then be rendered in DashboardMainApp and DashboardEntityApp. It can also accept a dashboard prop to see if it is being rendered in the main dashboard or entity dashboard, and render widgets accordingly.

• An issue may arise where a race condition causes an unexpected banner to appear. Given banners A and B (in that priority order), we might find that banner B resolves its state for displaying itself before banner A does. In this case, although we want A to take precedence, we'll end up showing B (maybe momentarily and then switching to A).
• An ideal way to address this without making things overly complicated is possibly by introducing an onReady prop function in the widget components, which they can use to let the parent component know when they are ready to be considered for prioritisation. The container component can maintain a local state of the widgets and their ready states, and use that to display the appropriate widget.

Let me know what you think. I'm absolutely open to any feedback/questions on the above. Of course, if the above feedback is applied, the estimate would need to notch up by 1 or 2 levels. Thank you!

from site-kit-wp.

Hi @nfmohit

Thanks for reviewing the IB.

I have a question regarding above comment added in IBR.

Keeping the above in mind, I'd recommend introducing a new assets/js/components/SetupCTAWidgets.js component. This component can include an array of setup CTA banner components to show, in the order of priority. The component can then iterate through each of the items in the array, and render the first one that does not return null.

1. Does that mean the previous points which talks about creating SetupCTAWidgets components and reusing it is no longer needed? As we will be iterating through components priority wise and display the component which does not return null.

2. While this approach looks good, can we ever have the requirement that we need to display two CTA widgets simultaneously. Consider a hypothetical scenario where we want to display one CTA widget from analytics module ( AudienceSegmentationSetupCTAWidget or ConsentModeSetupCTAWidget) and other one from ads module (AdsModuleSetupCTAWidget or AnyOtherCTAWidget ), not sure if this will be requirement, but in this case, the solution won't be able to address this.

3. Can you please elaborate on the onReady status mentioned in the last point of the comment? Sorry, I did not get it as to how exactly it is supposed to work.

from site-kit-wp.