Premium Analytics: document the widget catalog location and the Ads package - #52636
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. Mu Wpcom plugin:
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. |
b928f5d to
74d24a7
Compare
widgets doc: translations per record, the SDK a plugin's widgets import, the Ads package as the real consumer; README extension entry point
74d24a7 to
38d0bb7
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
one h3 per concern and short paragraphs instead of bullet lists
Code Coverage SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
Nikschavan
left a comment
There was a problem hiding this comment.
Thank you, the changes look good.
| The mechanics behind it: | ||
| ### The contract | ||
|
|
||
| The action hands over the `Widget_Type_Registry` being hydrated. `register_widget_types_from_manifest()` and `register_widget_type()` are what a plugin calls; both write to the main instance, which is that same registry in production. |
There was a problem hiding this comment.
This paragraph says both helpers write to the main instance and that $registry is there only for lookups, but register_widget_types_from_manifest() takes the registry as a third argument, and the Ads package passes it (class-analytics-dashboard.php#L132-L139). The VideoPress example above leaves it out, so the reference consumer and the doc show two different calls. Should a plugin pass $registry through, as Ads does?
| | The route's namespace and gate, hydration from the route | `tests/php/Widget_Modules_Test.php`, `tests/php/Analytics_Test.php` | | ||
| | The package's candidate policy and the runtime filter | `tests/php/Widget_Availability_Test.php` | | ||
| | The client's records read | `routes/use-widget-modules.test.ts` | | ||
| | The client's catalog loads per record, the resolver, the metadata preload | `tests/js/widget-module-i18n.test.tsx`, `wp-build-polyfills/tests/js/load-i18n-catalogs.test.js` | |
There was a problem hiding this comment.
This path needs a packages/ prefix to match its neighbours in the table; the file is at projects/packages/wp-build-polyfills/tests/js/load-i18n-catalogs.test.js.
louwie17
left a comment
There was a problem hiding this comment.
Thanks for updating the docs @retrofox, this looks good, just some small comments.
In docs/dashboard-sections.md the preview section still says PREVIEW_SECTIONS is traffic and insights, we have added subscribers now as well. Maybe we should add Ads already as well?
| array( 'textdomain' => 'jetpack-videopress-pkg' ) | ||
| array( | ||
| 'textdomain' => 'jetpack-videopress-pkg', | ||
| 'i18n_manifest' => plugins_url( 'i18n-manifest.json', __DIR__ . '/build/build.php' ), |
There was a problem hiding this comment.
Should we be adding a version to the i18n_manifest URL in this example? The Ads package does this with add_query_arg( 'ver', ... ), which is recommended, given the manifest could otherwise be served from the browser cache after an update.
Fixes WOOA7S-2189
Proposed changes
Step 4 of 4 of the widget-type extensibility stack (#52568, #52634, #52635). Documentation only: it describes the contract as it stands after steps 2 and 3.
The widgets doc
docs/dashboard-widgets.mdexplains where each widget module record says its translations live, and how the client loads a plugin's catalogs from there.A new section, "A real consumer: the Ads widgets", presents the
jetpack-adspackage (projects/packages/ads) as the reference. It includes how the package imports the dashboard: by one name,@automattic/jetpack-premium-analytics-sdk, the types-only js-package whose implementation this package registers at runtime from itspackages/sdkfacade (src/sdk-module.php), because wp-build keeps an import external only when it finds that package installed under its name and declared as a script module. The SDK also enters the vocabulary, the files table and whatWIDGET_API_VERSIONnames. The tests table and the list of what is not covered follow.The sections doc
docs/dashboard-sections.mdnow points at the package for the Ads layout and widgets, and says which copy of it WordPress.com loads: the one the Jetpack plugin bundles,jetpack-mu-wpcomlisting the package as a test-only dependency.The README
A new entry point explains how to extend the dashboard from another plugin: the two registration actions, the SDK the widgets import, who owns what, and the Ads package as the example to copy.
📸 Screenshot placeholder: the README's extension section, rendered
Agent rules
.agents/rules/widgets.mdno longer names a widget that moved. TheAGENTS.mdpointer to the chart tabs test changed in step 3, which removes that test.Related product discussion/links
Steps 1 to 3: #52568, #52634, #52635. Linear: WOOA7S-2183 for the stack and WOOA7S-2189 for this step.
Does this pull request change what data or activity we track or use?
No.
Testing instructions
Read
projects/packages/premium-analytics/docs/dashboard-widgets.mdand the README's extension section against the code of steps 2 and 3. Every file, function and dependency line they name exists on this branch.