Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement the loading state for the Audience Tile #8145

Closed
21 tasks done
techanvil opened this issue Jan 24, 2024 · 8 comments
Closed
21 tasks done

Implement the loading state for the Audience Tile #8145

techanvil opened this issue Jan 24, 2024 · 8 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

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 Implement the “partial data” badges #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 Add REST and datastore APIs for audience caching #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).

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/AudienceTiles/index.js:
    • Take in a new prop widgetLoading that is a boolean.
    • Update the loading constant to also be true when the widgetLoading is true.
  • 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.
  • 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.
    • Run an effect when the component is in view (using useInView hook) and the availableAudiencesSynced is false:
      • Call the newly created maybeSyncAvailableAudiences action.
      • Set availableAudiencesSynced to true afterwards.
    • Update the rendering logic in the following way:
      • When maybeSyncAvailableAudiences is false or either availableAudiences or configuredAudiences is not loaded (ie. undefined) return the AudienceTiles element with the widgetLoading prop being true.
      • If hasMatchingAudience is false, return WidgetNull.
      • Otherwise return AudienceTiles with the the widgetLoading prop being false.

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

  • Confirm the AudienceTile -> Loading story matches the figma designs (Note: the preview block heights have been adapted from the Figma design to better match the components they are simulating so they should be assessed on that basis.)

  • Enable the Audience Segmentation feature flag.

  • When the Audience Segmentation tiles load you should initially see the new loading state on any tiles when first resolving.

  • Configure some Audiences in your Audience Segmentation widget, then delete one/several of these audiences in Google Analytics.

  • Set the availableAudiencesLastSyncedAt in the wp_options -> googlesitekit_analytics-4_settings to 0.

  • Reload the Audience Segmentation section and these deleted audiences should be removed from your configured audiences.

Changelog entry

  • Show a loading state with placeholders for the content while data is being retrieved for an Audience Tile.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jan 24, 2024
@techanvil
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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

@techanvil techanvil assigned techanvil and unassigned techanvil Apr 9, 2024
@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Apr 9, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label May 24, 2024
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit May 28, 2024
@kuasha420 kuasha420 self-assigned this Jun 5, 2024
@kuasha420 kuasha420 removed their assignment Jul 4, 2024
@techanvil techanvil self-assigned this Jul 5, 2024
@techanvil
Copy link
Collaborator Author

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 techanvil assigned kuasha420 and unassigned techanvil Jul 5, 2024
@kuasha420
Copy link
Contributor

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 kuasha420 assigned techanvil and unassigned kuasha420 Jul 9, 2024
@techanvil
Copy link
Collaborator Author

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 techanvil assigned kuasha420 and unassigned techanvil Jul 9, 2024
@kuasha420
Copy link
Contributor

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 kuasha420 assigned techanvil and unassigned kuasha420 Jul 10, 2024
@techanvil
Copy link
Collaborator Author

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 ✅

@techanvil techanvil removed their assignment Jul 10, 2024
@ivonac4 ivonac4 removed the Next Up Issues to prioritize for definition label Jul 16, 2024
@benbowler benbowler self-assigned this Jul 16, 2024
@benbowler benbowler removed their assignment Jul 17, 2024
@techanvil techanvil self-assigned this Jul 18, 2024
@techanvil techanvil assigned benbowler and unassigned techanvil Jul 18, 2024
@benbowler benbowler assigned techanvil and unassigned benbowler Jul 23, 2024
@techanvil techanvil removed their assignment Jul 23, 2024
@wpdarren wpdarren assigned kelvinballoo and mohitwp and unassigned kelvinballoo Jul 24, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Jul 26, 2024

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-

Recording.1233.mp4

Audience Removal-

Recording.1234.mp4

@mohitwp mohitwp removed their assignment Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants