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

Implement the loading state for the Audience Tile

Open techanvil opened this issue 1 year ago • 3 comments

Feature Description

Implement the loading state for the Audience Tile.

See audience tiles > loading state in the design doc.


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

Acceptance criteria

  • An Audience Tile in the loading state should be displayed for each audience in the current list of selected audiences while the required data is loading, with preview blocks shown in place of the metric rows.
  • The loading state should follow the Figma design.
  • In terms of data loading, the following data should be retrieved and processed:
    • The "partial data" state should be resolved (see #8142).
    • The cached list of available audiences should be resynced if the cache is stale (i.e. if an hour or more has passed since the last sync. See #8486).
    • The list of selected audiences should be validated against the list of available audiences. If any of the selected audiences are no longer available, they should be removed from the selection.
    • The metrics should be retrieved for the tile(s).

  • Note that if an audience is removed from the selection, its tile may be replaced by a placeholder tile - however this is handled via #8146 and doesn't need to be addressed here.
  • Additionally if all audiences are removed from the selection, the tiles will be replaced by the "no audiences" banner, but again this is handled separately (see #8155), and doesn't need addressing here.

Implementation Brief

  • [ ] In assets/js/modules/analytics-4/datastore/audiences.js:
    • [ ] Create a new action maybeSyncAvailableAudiences:
      • [ ] Get the availableAudiencesLastSyncedAt Analytics Module settings value using getAvailableAudiencesLastSyncedAt selector.
      • [ ] Check whether audience cache needs resync using the availableAudiencesLastSyncedAt timestamp and run the syncAvailableAudiences if so.
      • [ ] Get the availableAudiences and configuredAudiences using their respective get** selectors.
      • [ ] Remove any configuredAudiences that are no longer available in availableAudiences.
  • [ ] In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/index.js:
    • [ ] Create a new state, availableAudiencesSynced using useState hook which is initially false.
    • [ ] Get the syncAvailableAudiences action from Analytics module using useDispatch hook.
    • [ ] On mount, using useMount hook:
      • [ ] Call the newly created maybeSyncAvailableAudiences action.
      • [ ] Set availableAudiencesSynced to true afterwards.
    • [ ] Update the logic for rendering AudienceTiles when availableAudiencesSynced is true in addition to the current hasMatchingAudience. ie. if ( availableAudiencesSynced && hasMatchingAudience ) { ...
  • [ ] In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/AudienceTile/index.js:
    • [ ] Update the loading state preview to match Figma design. This can be located under the TODO: Loading states will be implemented as part of https://github.com/google/site-kit-wp/issues/8145. comment.

Test Coverage

  • Add test coverage for newly added action.
  • Add test coverage for the newly introduced logic for AudienceTilesWidget component.
  • Update any failing tests.

QA Brief

Changelog entry

techanvil avatar Jan 24 '24 17:01 techanvil

I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one

techanvil avatar Mar 28 '24 14:03 techanvil

The audience caching aspect of the design doc has been sufficiently finalised, and I've moved this back to AC.

techanvil avatar Apr 05 '24 14:04 techanvil

Hey @kuasha420, thanks for drafting this IB.

  • [ ] Update the logic for rendering AudienceTiles when availableAudiencesSynced is true in addition to the current hasMatchingAudience. ie. if ( availableAudiencesSynced && hasMatchingAudience ) { ...

Hmm, if I understand correctly this will result in nothing being displayed for the tiles while maybeSyncAvailableAudiences() is running. This means there would be a layout shift when the action has finished and we proceed to show the tiles (or the no audiences banner, etc).

However this layout shift is something we want to avoid - we should instead be showing the tiles for the currently configured selection in their loading state while the sync is happening, in the assumption that the most likely path is for the audiences to still exist so we can show the tiles without a layout shift occurring. Of course if one of the edge cases occurs then we'll show the appropriate component and there may be a shift at that point, but we want to optimise for the UX for the most likely happy path view.

techanvil avatar Jul 05 '24 11:07 techanvil

Thank you for the review and raising this great point about layout shift. We definitely want to optimize the layout shift for the happy path. I've updated the IB accordingly. Please, let me know what you think.

Cheers.

kuasha420 avatar Jul 09 '24 08:07 kuasha420

Thanks @kuasha420, that's looking good. A couple of last points:

  • I've tweaked the IB to address a couple of minor copy/paste level issues, please review the recent edit and make sure it's correct.
  • I think the useMount() should in fact be a useEffect() as we probably want to use useInViewSelect() to select the values for availableAudiencesLastSyncedAt, availableAudiences and configuredAudiences, and only dispatch maybeSyncAvailableAudiences() when they are all defined, in order to be in-view compliant. If that SGTY, please update the IB accordingly.

techanvil avatar Jul 09 '24 12:07 techanvil

Thanks @techanvil. The Edits are perfect. Thank you. I've also updated the use of useMount with a regular effect. I'm not sure if this is necessary, as the action will only call API once an hour and the effect doesn't depend on the other values that are being selected with useInViewSelect but it makes sense for consistency. Let me know what you think. Cheers.

kuasha420 avatar Jul 10 '24 12:07 kuasha420

Thanks @kuasha420! I do think it's worthwhile in order to avoid making any requests while the widget is not in-view, which is our preferred pattern. That update LGTM.

IB :white_check_mark:

techanvil avatar Jul 10 '24 13:07 techanvil

QA Update ✅

  • Tested on main environment.
  • Verified that the implemented loading state match with the Figma design.
  • Verified that loading state appears when user change audience tiles from the widget.
  • Verified that the list of selected audiences validated against the list of available audiences. If any of the selected audiences are no longer available, they gets remove from the selection.
  • Verified the metrics gets retrieved for the tile(s).

Loading state-

https://github.com/user-attachments/assets/c61b522c-70d1-46d0-a513-b2bb6ac88e02

Audience Removal-

https://github.com/user-attachments/assets/9b641480-8815-40e1-9293-a30956c8c6b1

mohitwp avatar Jul 26 '24 10:07 mohitwp