WOOA7S-2211: Premium Analytics: Show the report empty state on every report - #52900
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Premium Analytics plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 14 files. Only the first 5 are listed here.
1 file is newly checked for coverage.
|
5ff413d to
36d1687
Compare
Nikschavan
added
[Status] Needs Review
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Nice catch on the reports missing the empty state! @Nikschavan I left a few inline notes.
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the updates! @Nikschavan Tested well. 👍 I left a few minor inline notes, none of them blocking.
| const dateFilters = useReportDateFilters( ROUTE_FROM ); | ||
| const tableIsLoading = records.table.isLoading || records.table.isFetching; | ||
| const { getLabel } = REPORTS.locations; | ||
| const showMap = !! countryFilter || records.table.rows.length > 0; |
There was a problem hiding this comment.
Should the map stay mounted while the table loads? I noticed that opening Regions for the first time unmounts it, so a Hide map the user picked comes back expanded.
| const showMap = !! countryFilter || records.table.rows.length > 0; | |
| const showMap = !! countryFilter || records.table.rows.length > 0 || records.table.isLoading; |
There was a problem hiding this comment.
Thank you, the map now stays mounted while the rows load, so a hidden map stays hidden when Regions opens for the first time. I used your suggestion as it was and added a test for the first load.
| { records.isLoading ? ( | ||
| <Spinner /> | ||
| ) : ( | ||
| { ( hasAllPostsFollowers || hasPostRows ) && ( |
There was a problem hiding this comment.
Not blocking, but would it be worth a shared helper for "is the report empty"? This page and Locations each re-derive what the table decides from its own view.filters, so they could drift.
There was a problem hiding this comment.
I kept this PR to the empty state itself. A shared helper would need to know about the country filter that Locations keeps in the table view, so I would like to look at it in a follow-up, where it can cover both pages.
| onChangePageItems?.( pageItems ); | ||
| }, [ onChangePageItems, pageItems ] ); | ||
|
|
||
| // A filter the API applies server-side can scope the rows to none, and the table carries the control that clears it. |
There was a problem hiding this comment.
Thank you, I rewrote the comment in report-error-state.tsx. I also removed the page comments that still named the empty prop, in Emails and Tags and in the same text on Annual insights, Authors and Comments.
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
Read the whole diff and ran the package's Jest suite, tsgo typecheck and ESLint on the changed files at 8ec9bdb; all pass. Moving the empty state into the two tables is the right fix, the eight table-level cases cover what the page tests used to, and I checked the loading claims against useStatsQuery: it folds isLoading into "pending, or placeholder data refetching", and the report queries set no placeholderData, so a period change does clear the rows and show the bare spinner, while a same-period revalidation only sets isFetching. No blockers; one changelog item and one consistency suggestion (inline).
Missing plugin changelog entries. This is a user-visible fix in packages/premium-analytics, so it needs entries in the plugins that surface it: plugins/jetpack (bugfix), plugins/premium-analytics (fixed) and plugins/wpcomsh (fixed, via jetpack-mu-wpcom), the same set the recent Premium Analytics PRs carried (#52714, #52747, #52690, #52870). The package entry's wording works for all three.
Checked and clean: the early return sits after every hook so view survives the empty state; view.filters correctly keeps the table for Locations' country filter; the four Comment followers combinations (rows/no rows × site-wide followers/none) each render sensibly; ReportEmptyState has no consumer outside this package, so dropping the export is safe; the empty prop has no remaining caller.
| /** Whether rows for the current params are still loading. */ | ||
| isLoading?: boolean; | ||
| /** Whether the rows on screen are revalidating; ignored while there are none, so an empty report keeps its empty state. */ | ||
| isFetching?: boolean; |
There was a problem hiding this comment.
Now that isFetching is the way a table shows a revalidation, only six pages pass it (clicks, downloads, posts, search-terms, locations, videos). The other nine (authors, utm, referrers, comments, emails, earnings, tags, annual-insights, comment-followers) pass isLoading alone, so a same-period refetch shows no indicator there. Those nine never OR'd isFetching in on trunk either, so this is not a regression, but since this PR settles the interface it would be cheap to pass isFetching={ records.isFetching } on them too and have all fifteen reports behave the same. Fine as a follow-up if you'd rather keep this one to the empty state.
There was a problem hiding this comment.
Thank you, I passed isFetching from the other nine reports as well, so all fifteen now show the table's loading state while rows on screen refresh. I also added the plugin changelog entries for Jetpack, Premium Analytics and WordPress.com Site Helper.
Every report now shows the full-page empty state when it has no rows, including the reports without a period picker, which get copy that does not mention a time period. The records and drilldown tables render it themselves, with a spinner in place of the toolbar until the first rows arrive.
A settled empty state stayed on screen while the next period loaded, because the stats queries keep the previous empty rows as placeholder data. Locations counts a refetch as loading only when rows are on screen, so a same-period refetch of an empty report keeps the empty state.
…ount The tables take isFetching apart from isLoading, so a same-period refetch keeps an empty report's empty state, while rows on screen still show the loading state. Comment followers shows the All Posts card whenever it has a count, even with no per-post rows.
…te API The records table keeps its controls when a view filter is set, replacing the keepWhenEmpty prop. The unused empty prop and the public ReportEmptyState export go, the period context defaults to false outside a date-filtered layout, and the loading spinner uses Stack.
…rows load Pass isFetching from the nine reports that did not, so a same-period refetch shows the table loading state on all fifteen. Keep the Locations map mounted during the first load so a hidden map stays hidden. Drop comments that named the removed empty prop, and add the plugin changelog entries.
…e case The drilldown table had no test that failed when isFetching stopped reaching DataViews with rows on screen. The records-table case for a settled empty state going to loading failed for the same reason as the first-load case, so it is removed.
8ec9bdb to
4c20d3c
Compare
github-actions
Bot
added
[Plugin] Jetpack
dognose24
left a comment
dognose24
left a comment
There was a problem hiding this comment.
Re-reviewed at 4c20d3c. Everything from the last round is in: the three plugin changelog entries, isFetching on all fifteen reports, the map staying mounted while rows load (with its test), and the comments that named the removed empty prop. Ran the package's Jest suite (including the two extra timezones) and the tsgo typecheck on this head: all pass.
Two non-blocking suggestions inline, both about what the tables do when they mount with isFetching already true. Fine as a follow-up.
| data={ pageItems } | ||
| getItemId={ getItemId } | ||
| isLoading={ isLoading } | ||
| isLoading={ isLoading || isFetching } |
There was a problem hiding this comment.
[suggestion] When the table mounts with isFetching already true, DataViews treats it as a first load: it renders no table, then a spinner after the delay, until the refetch finishes. That is what happens when someone comes back to a report whose cached rows have gone stale (stale after 5 minutes, kept for 10), so the cached rows stay hidden instead of showing while they refresh.
I checked both paths with a throwaway test. Mounting with rows and isFetching renders no rows. Rows already on screen that start refetching stay put, with the table aria-busy, and dim after the delay.
Six reports already did this on trunk by OR-ing isFetching into isLoading; passing it from the other nine, which I suggested last round, extends it to them. One way out is to leave isFetching out of the first render, so DataViews sees the rows before it sees the loading flag. ReportDrilldownTable has the same line. Happy for this to be a follow-up.
There was a problem hiding this comment.
Thank you, the tables now pass isFetching to DataViews only after their first render. A report that opens with stale cached rows now shows them right away, and the rows get the busy state once the refetch starts. ReportDrilldownTable does the same through a shared hook.
| it( 'shows the table loading state while rows on screen revalidate', () => { | ||
| mountRows( rows, { isFetching: true } ); | ||
|
|
||
| expect( screen.getByRole( 'searchbox' ) ).toBeInTheDocument(); | ||
| expect( screen.queryByText( 'Maharashtra' ) ).not.toBeInTheDocument(); | ||
| } ); |
There was a problem hiding this comment.
[suggestion] This mounts the table with isFetching already set, so the rows were never on screen: it pins the mount case from my other comment rather than the revalidation the name describes. For rows that are on screen, a render followed by a rerender with isFetching shows them still there with the table aria-busy, which is probably the behaviour worth pinning. The matching case in report-drilldown-table.test.tsx is written the same way.
There was a problem hiding this comment.
Thank you, I split the test into two cases in both table tests. One case mounts with isFetching and checks that the rows show. The other case renders the rows first, then rerenders with isFetching, and checks that the rows stay and that the table is aria-busy.
github-actions
Bot
added
[Status] UI Changes
Fixes WOOA7S-2211
Proposed changes
ReportRecordsTableandReportDrilldownTable, so every report gets the empty state from the table it already renders, and a new report cannot miss it.ReportPageLayouthas date filters. The reports without a period picker get "We couldn’t find any results." instead.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
fix/pa-report-empty-state-all-reports. A new site with no traffic is the easiest way to see the reports without a period picker empty.I checked the Referrers loading and empty states, and the Tags & categories empty state, on my local site. Earnings is not available on my site, so unit tests cover it.