Repository navigation
Premium Analytics: port Stats File Downloads widget - #50050
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! 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. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Code Coverage SummaryCoverage changed in 1 file.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
🤖 Review-cycle summary —
|
| Source | Item | Resolution |
|---|---|---|
| CI | Storybook tests failing — LeaderboardChart needs GlobalChartsProvider | Added withChartTheme decorator to EmptyState + WithMockRows stories (commit 48899fd) |
| claude[bot] | category "traffic" should match sibling Stats widgets | Fixed widget.json category "traffic" → "stats" (commit 18b56ca) |
| claude[bot] | WithComparison story renders identically to Default | Added docs note explaining file downloads has no PoP comparison view by design (commit 18b56ca) |
| claude[bot] | storyWidgetType trimmed object vs widgetDefinition | Acknowledged — current pattern avoids TS friction; devices widget uses same approach |
| claude[bot] | currentShare relative to max row, not total | Acknowledged — intentional, matches Calypso leaderboard behavior |
| claude[bot] | empty Stats mock fallback shape | Noted for future additions; no code change needed |
Unaddressed human comments: None.
CI: all required checks passing on commit 18b56ca.
chihsuan
left a comment
There was a problem hiding this comment.
Thanks @dognose24
I noticed that the focus style doesn't looks good. The focus outline gets clipped but I think this would need a general solution instead of fixing it in the widget so we can create a follow-up for it.
Also left a few inline comments but overall looks good.
Thanks! Worth separating the two patterns here, since they're intentionally different:
This also lines up with the direction called out (p1782509121752509-slack-C06FSDTSN82) The goal is to move widget-specific code out of That said. if the leaderboard + comparison-mapping here ends up largely duplicating in many widgets, that's a real signal that we should extract as a toolkit primitive/helper. |
…-stats-module-file-downloads # Conflicts: # projects/packages/premium-analytics/packages/widgets-toolkit/src/stories/mocks/register-stats-mocks.ts
…-stats-module-file-downloads # Conflicts: # projects/packages/premium-analytics/src/dashboard-layout.php
| <span>{ __( 'File downloads', 'jetpack-premium-analytics' ) }</span> | ||
| </span> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Should we change the value ofpresentation and remove this so host owns the title/header?
There was a problem hiding this comment.
I would keep this as-is for now. This widget intentionally uses presentation: "full-bleed" and owns its title/header, matching the direction we have been aligning with Locations/UTM/etc. Having these Stats widgets follow the same composition also gives us better control over consistent spacing, title/icon treatment, and state layouts. If we move title/header ownership back to the host, I think that should be a broader widget-root/guideline change rather than changing only File Downloads here.
There was a problem hiding this comment.
One extra note: I also expect Top Posts to move toward this same full-bleed/title-owned pattern when we align the remaining Stats widgets. So the intent here is consistent header/title/icon control across Stats widgets rather than File Downloads being a one-off.
There was a problem hiding this comment.
IMO, It feels a bit fragile to define a custom title/header here rather than reusing the widget's built-in header. If the core widget header ever changes, these custom header widgets could end up with inconsistent styles.
That said, no worries for now! This looks like an upstream issue that @retrofox is already working on over in WordPress/gutenberg#79246, so we should be in good hands.
There was a problem hiding this comment.
That said, no worries for now! This looks like an upstream issue that @retrofox is already working on over in WordPress/gutenberg#79246, so we should be in good hands.
Yeah, it should be, and we will figure out a better way to handle it all together.
There was a problem hiding this comment.
I think It's fragile as well. Using the title to represent the local state doesn't align with the API's design.
There was a problem hiding this comment.
WordPress/gutenberg#79735 is the first step in this
| .join( ' ' ); | ||
| } | ||
|
|
||
| function getFileDownloadsErrorMessage( error: unknown ) { |
There was a problem hiding this comment.
No other Stats widget carries this error-parsing cluster (toStatusNumber/getErrorStatus/getErrorText). Is this specific required for file downloads widget? Or I'm curious if we should handle this in data level so it can be reused for all Stats widget (maybe a follow-up).
There was a problem hiding this comment.
Yes, this is currently File Downloads-specific. The API returns a 404 for Jetpack sites where file download stats are not available, and we need to surface a more precise message than the generic load failure. I agree this would be better as a reusable data-layer error reason if/when more Stats widgets need it. UTM, Devices, and other advanced Stats features can also have API-side permission/availability controls, so normalizing this in the data layer sounds like a good follow-up.
There was a problem hiding this comment.
Opened a separate Premium Analytics follow-up for normalizing API availability/permission error reasons in the data layer. That should let widgets like File Downloads, UTM, Devices, and Top Platforms render consistent unavailable/error states without each widget parsing raw status/message fields.
| rows={ rows } | ||
| isLoading={ showLoading } | ||
| isError={ isError } | ||
| withComparison={ hasComparison } |
There was a problem hiding this comment.
top-posts gates on hasComparison && primaryRows.some(row => previousViewsByHref.has(row.href)), but here it's withComparison={ hasComparison } unconditionally.
When the comparison report has no overlapping rows, every previousValue falls back to 0 and calculateDelta(value, 0) returns 100 , so each row shows a fabricated +100%.
Should this defer comparison the way top-posts does when nothing overlaps?
There was a problem hiding this comment.
Good catch. I updated this to mirror top-posts: the comparison UI is now enabled only when at least one primary row has a matching comparison row, so missing comparison rows are no longer treated as real 0 values. I also checked nearby widgets: top-posts already has this guard, while UTM/Search Terms/Locations appear to have the same missing-comparison-as-zero pattern and should be aligned in their own PRs/follow-up rather than bundled into this File Downloads PR.
There was a problem hiding this comment.
Opened a Premium Analytics follow-up to audit and align this across the remaining Stats widgets. Keeping this PR scoped to File Downloads: Top Posts already has this guard, and the follow-up will cover the other row-level comparison widgets so they do not treat missing comparison rows as real zero values.
…-stats-module-file-downloads # Conflicts: # projects/packages/premium-analytics/packages/widgets-toolkit/src/stories/mocks/register-stats-mocks.ts # projects/packages/premium-analytics/src/dashboard-layout.php # projects/packages/premium-analytics/tests/php/Dashboard_Layout_Test.php
chihsuan
left a comment
There was a problem hiding this comment.
Thanks @dognose24 Looks good. Let's keep iterate this.
Fixes WOOA7S-1498
Proposed changes
Ports the Jetpack Stats File Downloads module into Premium Analytics as the
jpa/file-downloadswidget.Default,WithComparison, andWidgetDashboardWithWidgetStorybook stories.jpa/file-downloadsinto the default dashboard layout at 1x2.Related product discussion/links
Does this pull request change what data or activity we track or use?
No. This reads existing Jetpack Stats file-download data through the existing Premium Analytics stats data layer.
Testing instructions
Run:
In Storybook:
Packages/Premium Analytics/Widgets/FileDownloads.Default,WithComparison, andWidgetDashboardWithWidgetrender correctly.WithComparison.On a Jetpack-connected test site with file-download stats: