UX/UI issue on graphs when in Gathering Data state

wpdarren commented 2 years ago

Bug Description

While testing #4697 I noticed an odd UX/UI behaviour. The gathering data… loads slightly early when the loading animation is running for the graphs i.e. All traffic and Search Traffic widgets. See note from Asvin on 4697 re. needing further investigation. Screencast here (this is on entity, but is the same on main dashboard).

https://user-images.githubusercontent.com/73545194/158388495-73bab717-e439-48db-8e2e-75cfb4788058.mp4

The same issue occurs on any graphs on module pages.

Steps to reproduce

Enable zeroDataStates feature flag on the tester plugin.
Make sure you force data to 'Gathering' for search console and analytics.
Go to the Site Kit dashboard and observe details above.

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

Acceptance criteria

The "gathering data" overlay for any Google charts should load at the same time as the charts itself.
- This may need to rely on some sort of "loaded" callback/hook from the charts JS library in order to work. If that does not exist or is otherwise infeasible, we will have to research another approach.

Implementation Brief

Update `react-google-charts` version

Update NPM dependency react-google-charts to version 4.0.0, which provides the onLoad prop.

Fix the chart

In the GoogleChart component, pass the onLoad prop to the Chart component.
The onLoad callback should set some local component state, e.g. isChartLoaded.
Add isChartLoaded to the condition to render GatheringDataNotice. https://github.com/google/site-kit-wp/blob/3532d36abc6e50f61b7ce7aa63a0499a319b3a8f/assets/js/components/GoogleChart.js#L263-L264

Fix Storybook

The new react-google-charts version will break Storybook stories which display charts.
Removing the script tag that loads https://www.gstatic.com/charts/loader.js in .storybook/preview-head.html appears to fix it. https://github.com/google/site-kit-wp/blob/3532d36abc6e50f61b7ce7aa63a0499a319b3a8f/.storybook/preview-head.html#L15
Apply this fix, and ensure that all stories are fixed.
Ensure the VRT suite passes.

Test Coverage

No new tests needed.

QA Brief

Follow the Steps to reproduce in the Bug Description.
Verify the "gathering data" notice appears in sync with the chart loading.
Check that Storybook stories with charts are still working, e.g.
- Line chart: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/adsense-module-components-module-overview-widget--loaded
- Line chart in DataBlock Sparkline: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/adsense-module-components-dashboard-summary-widget--loaded
- Line & pie charts: https://google.github.io/site-kit-wp/storybook/develop/?path=/story/analytics-module-components-dashboard-all-traffic-widget--loaded

Changelog entry

Fix gathering data message appearing too early on chart components.

eugene-manuilov commented 2 years ago

IB ✔️

wpdarren commented 2 years ago

QA Update: ✅

Verified:

The "gathering data" notice appears in sync with the chart loading. This was tested on desktop and mobile.
The Storybook stories with charts are still working.
Checked that the graphs loading animations look expected on a site with data.
Checked that the graphs loading animations look as expected on a site with zero data.

https://user-images.githubusercontent.com/73545194/161740160-c3b70eae-05f0-48f9-9296-35d8d0aadd0a.mp4

aaemnnosttv commented 2 years ago

QA ⚠️ ❗

@techanvil @felixarntz Turns out the changes made to GoogleChart introduces a flicker of loading state when switching between different chart instances due to the initial "unloaded" state which is visible on the Search Funnel widget.

This is only visible when switching to/from GA charts since the SC charts share the same instance.

felixarntz commented 2 years ago

@wpdarren @techanvil @aaemnnosttv Thanks for catching and looking into this. Since it's in the release and we don't have much time left on the investigation, I'd suggest we cap this on Monday (somewhere around end of day Europe, start of the day US) if not completed by then and make a decision.

I'm a bit wary about the React chart update causing potentially further problems, as it has been a recurring issue for us that these updates haven't been exactly smooth several times. Unless we're confident this update isn't causing other issues, I'd prefer reverting this from the main release branch and putting it back into develop instead to give it more time for testing. The original problem that this issue solves is not severe enough to "justify" potential breakage.

techanvil commented 2 years ago

@wpdarren thanks for the catch, sorry this one slipped past me at the time.

@felixarntz @aaemnnosttv, this is indeed a result of the upgrade in react-google-chart version, as opposed to changes we've made to the GoogleChart component itself.

I've found a possible alternative approach for the fix that doesn't require bumping the version. It feels mildly hackish, and would need a bit more testing to ascertain whether it's a viable solution, but I have created a PoC branch to illustrate: https://github.com/google/site-kit-wp/compare/main..bug/4945-fix-poc

It would be nice to figure out a more "correct" solution and keep the upgraded react-google-chart, but I suspect this might be out of reach for a release on Monday.

wpdarren commented 2 years ago

@techanvil no problem, I've been wondering why I didn't notice this while testing the original ticket because it jumped out to me straight away when release testing. 🤔 Currently this is in QA, can you confirm its ready for us to look at?

techanvil commented 2 years ago

@techanvil no problem, I've been wondering why I didn't notice this while testing the original ticket because it jumped out to me straight away when release testing. thinking Currently this is in QA, can you confirm its ready for us to look at?

Thanks @wpdarren, I've moved this back to Execution while we determine the fix.

aaemnnosttv commented 2 years ago

@techanvil would you please open a PR with your suggested workaround?

Worst case, we will revert this issue back and punt it to a future release.

techanvil commented 2 years ago

Thanks @aaemnnosttv, I have created a PR with the workaround: #5069 .

I've been looking into the issue more today, although without substantial progress. As mentioned, the issue evidently relates to the react-google-charts version, but I've not managed to pinpoint the change that makes the difference. I suspect it might relate to the change from using react-load-script in v3 to using their own loader in v4, but there are other changes to consider too.

It seems that we need to investigate upgrading react-google-charts as a separate issue, as and when we wish to upgrade...

wpdarren commented 2 years ago

QA Update: ✅

Verified:

The "gathering data" notice appears in sync with the chart loading.
The issue reported is fixed.
The graphs on main and entity dashboard no longer flicker. I tested this with a site that has live data and zero data. This is with the zero data flag enabled and disabled. Checked graphs on 7, 14, 28 and 90 days. Desktop and mobile.
Made sure the graphs function as expected.

https://user-images.githubusercontent.com/73545194/162804133-f39b5cd3-da76-4975-bff9-f86dff9fa986.mp4

google / site-kit-wp