site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

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

Open techanvil opened this issue 1 year ago • 6 comments

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

  • Audience Segmentation components which are rendered within the scope of the Audiences Widget Area should be updated to use the useInViewSelect() hook in place of useSelect() for any selectors which use a resolver or result in a resolver firing via another selector.
  • An observable result of this is that the widgets in the Audiences Widget Area should not trigger any API requests when it's not in-view (for the area to be considered in-view, the Traffic section should be the current section on the dashboard).

Implementation Brief

  • [ ] Update the following uses of useSelect to use useInViewSelect:
    • [ ] assets/js/modules/analytics-4/components/audience-segmentation/dashboard/InfoNoticeWidget/index.js: https://github.com/google/site-kit-wp/blob/e12d144418423b637879d8dd0f0d181acaa89b1b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/InfoNoticeWidget/index.js#L43-L45
    • [ ] assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js, NOTE: the selectors will change as part of the pivot report structure refactoring, however the same useSelects should updated once this work has been completed.: https://github.com/google/site-kit-wp/blob/e12d144418423b637879d8dd0f0d181acaa89b1b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L219-L221 https://github.com/google/site-kit-wp/blob/e12d144418423b637879d8dd0f0d181acaa89b1b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L232-L241 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L67-L69 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L73-L75 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L98-L100 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L115-L119 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L147-L152 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L173-L178 https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget.js#L199-L204
    • [ ] assets/js/modules/analytics-4/components/audience-segmentation/dashboard/ChangeGroupsLink.js: https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/ChangeGroupsLink.js#L38-L40
    • [ ] assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTile/index.js: https://github.com/google/site-kit-wp/blob/0933d6c17774e59fed8990f5893a6c6c0b672eeb/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTile/index.js#L83-L106

Test Coverage

  • No additional test coverage required.

QA Brief

Changelog entry

techanvil avatar Jun 14 '24 17:06 techanvil

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!

nfmohit avatar Jun 26 '24 08:06 nfmohit

@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

zutigrm avatar Jun 26 '24 09:06 zutigrm

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

nfmohit avatar Jun 26 '24 09:06 nfmohit

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?

nfmohit avatar Jun 26 '24 20:06 nfmohit

I missed that one thanks @nfmohit

benbowler avatar Jun 27 '24 07:06 benbowler

Thanks @benbowler ! IB LGTM 👍 ✅

nfmohit avatar Jun 30 '24 01:06 nfmohit

QA Update ✅

  • Loaded the dashboard at the monetization section, when slowly scrolling to the traffic section, the Audience Segmentation section loads as it comes into view.

    https://github.com/user-attachments/assets/4456c159-6457-43d3-b752-04b833a555e5

  • Did a more detailed test with the dev tools Network tab and no report or *audience endpoints will be called until we scroll the audience segmentation widget into view.

    https://github.com/user-attachments/assets/f33c1e70-0224-4cff-8bbd-6dc8ab8a5580

kelvinballoo avatar Aug 30 '24 13:08 kelvinballoo

@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

aaemnnosttv avatar Sep 05 '24 18:09 aaemnnosttv

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?

nfmohit avatar Sep 05 '24 18:09 nfmohit

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

techanvil avatar Sep 06 '24 14:09 techanvil

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 avatar Sep 06 '24 15:09 aaemnnosttv

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.

aaemnnosttv avatar Sep 06 '24 17:09 aaemnnosttv

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

techanvil avatar Sep 06 '24 17:09 techanvil

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!

nfmohit avatar Sep 06 '24 17:09 nfmohit

Thanks Nahid, no worries!

techanvil avatar Sep 06 '24 17:09 techanvil