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.25k stars 291 forks source link

All Traffic Widget: Don't show Dimensions tabs when no data is available for Analytics #2675

Closed tofumatt closed 3 years ago

tofumatt commented 3 years ago

When the All Traffic Widget doesn't have any data to show, the tabs that allow the user to select their dimension should not appear, because there's no data to filter. Currently, the tabs appear even when isZeroData( report ) is truth-y, but they should only be shown when isZeroData( report ) is false-y.

Screenshots

image


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

Acceptance criteria

Implementation Brief

After a call with @felixarntz, @aaemnnosttv, and @eugene-manuilov, we decided the best solution is to move all selectors for the DashboardAllTrafficWidget children components into the DashboardAllTrafficWidget component itself, then pass the loading and data/report variables to the child components as props. Data in error (from the getErrorForSelector selector) should not be passed to children—instead, if the component has zeroData from the UserDimensionsPieChart component or an error state, one large "zero data" or "error" state should be rendered.

Test Coverage

Visual Regression Changes

QA Brief

Changelog entry

aaemnnosttv commented 3 years ago

Thanks @tofumatt – I think there are some details left to consider here.

This is a bit of a tricky UX problem as-is; the easiest solution to which would simply be to not to use ReportZero within the sub-components but that would be a big difference than it is now visually. Otherwise, it seems like the only solution would be to select data from all available dimensions and then only show ReportZero for the entire widget (when there really is zero data for any dimension) rather than specific sections; this sounds like too much from an API/data/quota perspective though. Widgets are supposed to only show this once for the whole widget anyways, not sure why we have it on both sides for this one 🤔

Let's talk about this one with @felixarntz on our call later since there are some considerations here that we need his input on.

felixarntz commented 3 years ago

@tofumatt Reviewing this, there is actually a benefit of still passing through each request error to its respective component: When e.g. selecting a pie slice, only the data on the left is refreshed. If an error happens there, it means only the UI for that pie slice (the left half of the widget specifically) should indicate the error state. It's unlikely that this will happen, but in that case the overall widget is still usable, so there shouldn't be a global error.

I suggest the following adjustment:

cc @aaemnnosttv @eugene-manuilov

tofumatt commented 3 years ago

@felixarntz Oh, good call. That makes sense; I've updated the IB 👍🏻

aaemnnosttv commented 3 years ago

IB looks good to me 👍

felixarntz commented 3 years ago

IB ✅ from my end, waiting for @eugene-manuilov's feedback

eugene-manuilov commented 3 years ago

IB :heavy_check_mark:

eugene-manuilov commented 3 years ago

Moving to the execution column and will work on it tomorrow morning.

aaemnnosttv commented 3 years ago

QA ✅

In no data state, the widget shows the same full-area ReportZero component as in the legacy widget

image