Conversation
Per Eder's design pass: the widget's payment status is a badge as on the report, and the two pending statuses read "Pending" with their reason in an info icon beside the badge, on both surfaces. The report's Status filter offers Pending once for both codes, and the CSV keeps the reason. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
dognose24
added
Enhancement
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
Bot
added
[Package] Premium Analytics
[Plugin] Jetpack
|
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. |
The status badge carries the colour now; a negative amount reads with its minus sign in the normal text colour, as on the report. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Code Coverage SummaryCoverage changed in 2 files.
|
An inline-flex wrapper blockified the badge, so a pending row's badge came out taller than the bare badges around it. The wrapper stays inline and only the info icon is an inline-level box. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Every status now sits in the same inline-flex wrapper, which blockifies the badge so it takes its line height plus padding, as the design draws it, rather than painting over a shorter line box. The pending row's icon shares that wrapper. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The info icon is the design system Popover the report chart help uses, which opens on a short hover, a click, or the keyboard. The components Tooltip waited on a long hover and ignored clicks. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…header tip Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The popover title is heading-sized and stays hidden, as in the widget header's tip; the reason opens the description instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the design pass! @dognose24 Tested well. 👍
I left a few inline notes, mostly around reusing the existing info tip and the tests.
| <Popover.Root modal="trap-focus"> | ||
| <Popover.Trigger aria-label={ detail } className={ styles.info }> | ||
| <Icon icon={ info } size={ 16 } /> | ||
| </Popover.Trigger> | ||
| <Popover.Popup className={ styles.popup }> | ||
| <Popover.Arrow /> | ||
| <VisuallyHidden render={ <Popover.Title /> }>{ detail }</VisuallyHidden> | ||
| <Popover.Description> | ||
| <span className={ styles.reason }>{ detail }</span> | ||
| { tooltip } | ||
| </Popover.Description> | ||
| </Popover.Popup> | ||
| </Popover.Root> | ||
| <Badge intent={ intent }>{ label }</Badge> |
There was a problem hiding this comment.
Could we reuse ReportChartHelp here instead of a second info tip? I noticed this one repeats its trigger reset and popup width with a different icon size (16 vs 20), so the two may drift as they get tweaked.
| // The same click-open tip as the widget header's info icon. | ||
| return ( | ||
| <span className={ styles.root }> | ||
| <Popover.Root modal="trap-focus"> |
There was a problem hiding this comment.
I wonder if modal="trap-focus" does anything here. Without a Popover.Close, base-ui treats the popup as non-modal, so Tab moves straight out and closes it. Should we drop modal, or add a hidden Close so the trap is real?
| @@ -0,0 +1,30 @@ | |||
| /* Inline-flex blockifies the badge, so it stands its full line height plus | |||
| * padding, as in the design, and every status comes out the same height. */ | |||
| .root { | |||
There was a problem hiding this comment.
Could .root and .reason be Stack and Text variant="heading-sm" from externals? With the tip shared, this stylesheet may not need to exist.
| // row keeps its reason, which the table shows in an icon. | ||
| getValue: row => { | ||
| const { label, detail } = getEarningsStatus( row.status ); | ||
| return detail ? `${ label } (${ detail })` : label; |
There was a problem hiding this comment.
Could this go through sprintf like build-csv.ts does for %s (Previous Period)? The parentheses and order are fixed here, so some locales can't translate the export string.
| getValue: row => getEarningsStatus( row.status ).label, | ||
| // The numeric code says nothing to a reader of the export; a pending | ||
| // row keeps its reason, which the table shows in an icon. | ||
| getValue: row => { |
There was a problem hiding this comment.
Worth a pending row in report-csv-exports.test.tsx? The Earnings history case only has Unpaid and Paid, so this branch never runs there.
| render( <EarningsStatusBadge status={ 3 } /> ); | ||
|
|
||
| expect( screen.getByText( 'Pending' ) ).not.toHaveAttribute( 'tabindex' ); | ||
| expect( screen.getByRole( 'button', { name: 'Missing tax info' } ) ).toBeInTheDocument(); |
There was a problem hiding this comment.
Could this also click the trigger and check the explanation, like report-chart-section.test.tsx does? Right now the description could be emptied without a test going red.
| it( 'keeps the pending reason beside the label, not in it', () => { | ||
| expect( getEarningsStatus( 3 ).detail ).toBe( 'Missing tax info' ); | ||
| expect( getEarningsStatus( 4 ).detail ).toBe( 'Invalid PayPal' ); | ||
| expect( getEarningsStatus( 0 ).detail ).toBeUndefined(); | ||
| } ); |
There was a problem hiding this comment.
nit: Should this one go? It reads the constant back from the map, and the badge test below already covers the reason.
| <EarningsStatusLabel status={ row.status } /> | ||
| <span className={ styles.amount }>{ formatMetricValue( row.amount, 'currency' ) }</span> | ||
| <span className={ styles.status }> | ||
| <EarningsStatusBadge status={ row.status } /> |
There was a problem hiding this comment.
Worth a list test with a pending row? earnings-history-list.test.tsx never asserts the status cell, so swapping the badge out would stay green.
| * WordAds payment statuses by code, ported verbatim from the Jetpack Stats WordAds | ||
| * `getStatus` map (wp-calypso client/my-sites/stats/wordads/earnings.jsx). |
There was a problem hiding this comment.
nit: Still verbatim? The pending labels now differ from Calypso's.
| * WordAds payment statuses by code, ported verbatim from the Jetpack Stats WordAds | |
| * `getStatus` map (wp-calypso client/my-sites/stats/wordads/earnings.jsx). | |
| * WordAds payment statuses by code, adapted from the Jetpack Stats WordAds | |
| * `getStatus` map (wp-calypso client/my-sites/stats/wordads/earnings.jsx), |
| Significance: patch | ||
| Type: changed | ||
|
|
||
| Ads: Show payment status as a badge in the Earnings History widget, and shorten the pending statuses to one word with the reason beside them. |
There was a problem hiding this comment.
nit: Worth a clause that negative amounts on the widget are no longer red? #52389's entry announced the red, so readers may look for it leaving.
2 participants
Part of WOOA7S-2160
Proposed changes
Eder's design pass on the Earnings History widget (p1790006682602199-slack-C06FSDTSN82): badges everywhere the payment status shows, and the pending explanation outside the badge.
EarningsStatusBadgethe Earnings report has used since Premium Analytics: show sponsored content and adjustment earnings on the Earnings history report #52437: Paid green, Unpaid red, Pending orange. The plain-textEarningsStatusLabelhad no other caller and is gone.Pending (Missing tax info).Not in this PR: the adjustments badge in the widget footer (#52526).
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Use a site with WordAds earnings, or the local
zz-tmp-pa-wordads-mock.phpmu-plugin, whose payload has Unpaid, Paid and one Pending (code 3) row.Pending (Missing tax info).-$2.50reads in the neutral text colour with its minus sign, as before.jp test js packages/premium-analyticspasses.🤖 Generated with Claude Code