Enhancement/12551 top earning#13035
Conversation
📚 Storybook for 334c5b8: 📦 Build files for 334c5b8:
🎭 Playwright reports for 334c5b8: |
|
Size Change: 0 B Total Size: 3.62 MB ℹ️ View Unchanged
|
tofumatt
left a comment
tofumatt
left a comment
There was a problem hiding this comment.
I left some questions and comments here; everything largely looks good but I think there's some improvements to be made before we merge this. Please let me know what you think 🙂
| ); | ||
| } ); | ||
|
|
||
| it( 'selects a widget whose eligibility resolves after the initial seeding, without re-seeding deselected ones', async () => { |
There was a problem hiding this comment.
What is "seeding" in this context, I don't understand. 🤔
There was a problem hiding this comment.
It was refering to the initially loaded/available widget, I updated it to be more clear
| // A widget gated on a store value that is not available when the panel | ||
| // opens, like a `pdf.isActive` that reads a not-yet-resolved setting. |
There was a problem hiding this comment.
Please don't use the word "gated"; it's the kind of language an LLM will generate but isn't as human-readable as something like: "A widget that appears when the panel opens."
I'm not sure what "like a pdf.isActive that reads a not-yet-resolved setting." means. Can you clarify this whole comment?
There was a problem hiding this comment.
Thanks. I expanded on all referenced comments so it is more clear
|
|
||
| openPanel(); | ||
|
|
||
| // The initial seeding runs with the gated widget still ineligible. |
There was a problem hiding this comment.
See above re: "seeding" and "gated".
Ineligible to what?
| queryByRole( 'checkbox', { name: /^Revenue$/ } ) | ||
| ).not.toBeInTheDocument(); | ||
|
|
||
| // The user deselects a seeded widget before the gate resolves. |
There was a problem hiding this comment.
Is this something like?
// The user deselects a widget with populated data before the check (to what?) resolves.
| registry.dispatch( CORE_UI ).setValue( 'adSenseLinked', true ); | ||
| } ); | ||
|
|
||
| // The late widget appears checked; the deselected one is not re-seeded. |
There was a problem hiding this comment.
What is the widget late for? 🤔
There was a problem hiding this comment.
It was refering to newly appeared widget/the one that doesn't show immediately. I clarified this and other comments with more details
| const topEarningGetData = jest.fn( () => | ||
| Promise.resolve( { data: null } ) | ||
| ); | ||
| // The Top earning pages widget gates PDF inclusion on the AdSense link, |
There was a problem hiding this comment.
Again: please don't use "gates" or "gated" as verbs. They're very specific to LLM-generated text and overly technical.
| function provideReports( { | ||
| main = MAIN_REPORT, | ||
| titles = TITLES_REPORT, | ||
| }: { main?: unknown; titles?: unknown } = {} ) { |
There was a problem hiding this comment.
Why are these unknown? Surely we know the types if we have defaults. 🤔
| import { MODULES_ADSENSE } from '@/js/modules/adsense/datastore/constants'; | ||
| import getPDFData from './getPDFData'; | ||
|
|
||
| type Registry = WPDataRegistry & GetPDFDataParams[ 'registry' ]; |
There was a problem hiding this comment.
What's with the GetPDFDataParams type here? It seems to include its own registry prop, this is a confusing type…
If we can't use the regular registry type here we should document why, but this seems wrong.
There was a problem hiding this comment.
GetPDFDataParams['registry'] is just the registry getPDFData expects - a normal registry plus resolveSelect (which the loader calls but isn't in the upstream WPDataRegistry type). WPDataRegistry & on top was excessive, I simplified it and added a note on why the regular type doesn't work on its own:
There was a problem hiding this comment.
This file is named reportOptions.ts but it exports a single function: getTopEarningPagesReportOptions.
Why not call it getTopEarningPagesReportOptions.ts? 🤔
| /** Report rows, the currency code, and the page titles, or `null` when cancelled. */ | ||
| data: { | ||
| rows: ReportRow[]; | ||
| currencyCode: string; | ||
| titles: Record< string, string >; | ||
| } | null; |
There was a problem hiding this comment.
It seems odd to return this in a data attribute; why not just return each property in an object directly rather than as an object inside this single property? 🤔
There was a problem hiding this comment.
Every PDF widget's getData has to return an object shaped { data, chartImages? } - that's the return type of getData in the WidgetPDFConfig interface (assets/js/googlesitekit/widgets/types.ts). It's not something specific to this widget; it's the contract all the PDF loaders share.
In PDFExportOrchestrator.tsx, after it calls getData, it reaches into the result and pulls result.data out to pass down to the widget's Component as its data prop (alongside chartImages):
I updated this to split the types so the nesting isn't confusing
|
Thanks @tofumatt PR udpated. E2E is failing for unrelated flakiness |
2 participants
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist