search

google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.23k stars 286 forks source link

Use the `useInViewSelect()` hook where applicable in the Audience Segmentation feature #8879

Closed techanvil closed 3 weeks ago

techanvil commented 3 months ago

Feature Description

The various Audience Segmentation components which are within the scope of an "active" InViewProvider should be updated to use the useInViewSelect() hook in place for useSelect() for any selectors which use a resolver or result in a resolver firing via another selector.

This is to ensure that we don't trigger unnecessary API calls and other unwanted side effects when the component is not considered in-view.

An "active" InViewProvider is one whose in-view state is not hardwired to true. In practical terms for the Audience Segmentation feature, this means the InViewProvider instances rendered within WidgetAreaRenderer. So, a simpler way to describe the requirement is for any of the components rendered in the Audiences Widget Area to use the useInViewSelect() hook as described above.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

nfmohit commented 3 months ago

Thank you for the IB, @benbowler !

  1. I don't think we need to update the following components as they are not a part of the widget area and are rendered at the top of the dashboard on top of any widgets:
    1. assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
    2. assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js
  2. We should consider updating the isPromptDismissed selector in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/InfoNoticeWidget/index.js as this is a part of the widget area and this selector invokes a resolver.
  3. We should consider updating the getDismissedItems and isAudiencePartialData selectors in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js.
  4. A minor nit, but in the test coverage section, if no test updates are needed, please mention such so that it looks like this was taken into consideration.

Please let me know what you think, thank you!

zutigrm commented 3 months ago

@benbowler @nfmohit Just to chip in, useInViewSelect has been modified to expect direct dependencies in #8789 which will be merged soon, it is worth to mention it as part of the IB for the current issue. I will ping about this in the group as well once the PR for the referenced issue is merged

nfmohit commented 3 months ago

Thank you @zutigrm ! I've added #8789 as a blocker for this issue. CC: @techanvil @benbowler

nfmohit commented 3 months ago

Thank you for the update, @benbowler . I see that the requirement to make changes in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupSuccessSubtleNotification.js wasn't removed. Was that left on purpose?

benbowler commented 3 months ago

I missed that one thanks @nfmohit

nfmohit commented 3 months ago

Thanks @benbowler ! IB LGTM 👍 ✅

kelvinballoo commented 1 month ago

QA Update ✅

aaemnnosttv commented 3 weeks ago

@nfmohit @techanvil did we miss an this? https://github.com/google/site-kit-wp/blob/ebcb7b7975da13bae3c2b91923706b4d68195805/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSelectionPanel/AudienceItems.js#L79-L133

nfmohit commented 3 weeks ago

Good catch @aaemnnosttv! It looks like we missed it 😮 Would you like us to address this via a follow-up PR, or create a new issue?

techanvil commented 3 weeks ago

Good catch @aaemnnosttv! It looks like we missed it 😮 Would you like us to address this via a follow-up PR, or create a new issue?

Hmm, actually this isn't a miss for the issue as specced. AudienceItems is rendered in AudienceSelectionPanel which is in turn rendered directly in DashboardMainApp (along with MetricsSelectionPanel):

https://github.com/google/site-kit-wp/blob/b6c7c59c9bece36c8b0138d2e3d4f05a2f09479a/assets/js/components/DashboardMainApp.js#L295-L297

That means it's not in a WidgetAreaRenderer, and will use the in-view state provided by Root's InViewProvider which is fixed to true, so there's no point using useInViewSelect().

https://github.com/google/site-kit-wp/blob/b6c7c59c9bece36c8b0138d2e3d4f05a2f09479a/assets/js/components/Root/index.js#L57-L64

So, it's not a miss for this issue as it stands. We could however look at improving our Selection Panels, maybe we can render them in their related WidgetAreaRenderer components so they do honour the in-view state. WDYT? @aaemnnosttv @nfmohit

aaemnnosttv commented 3 weeks ago

So, it's not a miss for this issue as it stands. We could however look at improving our Selection Panels, maybe we can render them in their related WidgetAreaRenderer components so they do honour the in-view state. WDYT? @aaemnnosttv @nfmohit

Thanks for the context @techanvil, I think your suggestion makes sense, but we could also potentially give the side panel it's own in-view state which could simply be tied to whether or not it is open or closed rather than putting it into a widget area and tying it to intersection observer, etc. The KM panel never made API requests to Analytics I don't think (unless perhaps a custom dimension was needed) but that is an on-demand action. The general principal behind the in view select was to avoid side effects that hit Google APIs from loading the dashboard all at once. Since the panel does source data about available audiences, and even reporting data, I think it makes sense that these should be guarded by the in view state where possible. I think available audiences needs to be requested to inform it's CTA or something else before it may be in view, but if so, this should be minimized as much as possible and ideally uses requests that are cached.

aaemnnosttv commented 3 weeks ago

Closing this out for now, we can follow up with anything else in new issues 👍

As mentioned above though, we should aim to avoid requests being made by selectors within the panel components when it's closed.

techanvil commented 3 weeks ago

Thanks @aaemnnosttv. Your suggestion to make the in-view state dependent on the open/closed state is a good one.

Actually, it got me wondering if we could take a simpler approach and make the rendering of the children in SelectionPanel conditional so they are only rendered into the DOM when the panel has been opened, so none of their rendering logic runs on page load. I gave it a quick spin, it works well on my laptop at its normal speed, but when I throttle the CPU the panel appears empty when it starts scrolling into view, so maybe this isn't such a good approach. Anyway, thought I'd mention it, food for thought and that.

The KM panel never made API requests to Analytics I don't think (unless perhaps a custom dimension was needed) but that is an on-demand action.

Actually, it does call getAvailableCustomDimensions() on page load, as per the comment this is a fix to prevent a flicker with the notice. This could be problematic if we make it an in-view select, although only with the combination of a very quick user action and a slow custom dimension sync.

https://github.com/google/site-kit-wp/blob/2919899c5e40e580142ccfc6c49e78b63e66a653/assets/js/components/KeyMetrics/MetricsSelectionPanel/CustomDimensionsNotice.js#L67-L71

I think available audiences needs to be requested to inform it's CTA or something else before it may be in view

That one has already been migrated to an in-view select, so we are OK on that front:

https://github.com/google/site-kit-wp/blob/2919899c5e40e580142ccfc6c49e78b63e66a653/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/ChangeGroupsLink.js#L36-L39

Anyhow - the main points you've raised are all good ones; maybe this issue should have gone a bit further in the first instance.

I've gone ahead created a new issue to follow up on this, which I've moved to ACR: https://github.com/google/site-kit-wp/issues/9312

nfmohit commented 3 weeks ago

So, it's not a miss for this issue as it stands. We could however look at improving our Selection Panels, maybe we can render them in their related WidgetAreaRenderer components so they do honour the in-view state. WDYT? @aaemnnosttv @nfmohit

Oops, that's correct. It looks like I made an abrupt judgement mistakenly confusing AudienceItems with AudienceTiles. Thank you for correcting and for addressing this, folks!

techanvil commented 3 weeks ago

Thanks Nahid, no worries!