Repository navigation
Conversation
jpa/toggle-group takes jpa/<name> icons; drops the chart-line copies
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
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! |
Code Coverage SummaryCoverage changed in 1 file.
|
This was referenced Oct 19, 2026
…icon-references Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # projects/packages/premium-analytics/docs/dashboard-widgets.md
the icon-reference form ships with the field alone; widgets move to widget.json in the follow-up, and the contract takes 1.15.0 after the registry landed at 1.14.0
retrofox
added
[Status] Needs Review
retrofox
marked this pull request as ready for review
October 19, 2026 19:07
chihsuan
approved these changes
Oct 20, 2026
Comment on lines
15
to
+16
| // `Option` carries no `icon`, so the icon-bearing fixtures name their own | ||
| // shape, the way the toolkit's `CHART_TYPE_ELEMENTS` are built. | ||
| type IconOption = Option & { icon: ReactElement }; | ||
| // shape: an element, or a `jpa/<name>` reference as `CHART_TYPE_ELEMENTS` carries. |
Member
There was a problem hiding this comment.
Could we drop the CHART_TYPE_ELEMENTS reference here? After the last commit it carries React elements again, so this comment (and the one in the stories file) now describes #53778's state.
Suggested change
| // `Option` carries no `icon`, so the icon-bearing fixtures name their own | |
| // shape, the way the toolkit's `CHART_TYPE_ELEMENTS` are built. | |
| type IconOption = Option & { icon: ReactElement }; | |
| // shape: an element, or a `jpa/<name>` reference as `CHART_TYPE_ELEMENTS` carries. | |
| // `Option` carries no `icon`, so the icon-bearing fixtures name their own | |
| // shape: an element or a `jpa/<name>` reference. |
| * @param elements - The options as the widget declares them. | ||
| * @return The options with every icon an element. | ||
| */ | ||
| export function resolveOptionIcons( elements: Option[] ): Option[] { |
Member
There was a problem hiding this comment.
Could resolveOptionIcons stay module-private? I couldn't find anything importing it, including the tests and #53778, so the export may read as public API nobody uses yet.
| * instead of showing a blank one. | ||
| * | ||
| * @param elements - The options as the widget declares them. | ||
| * @return The options with every icon an element. |
Member
There was a problem hiding this comment.
nit: Would it be worth rewording @return? An unresolved reference drops its icon, so not every option comes back with an element.
Suggested change
| * @return The options with every icon an element. | |
| * @return The options, with each resolvable icon reference replaced by its element. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of WOOA7S-2290
Proposed changes
jpa/toggle-groupaccepts theiconof an option as ajpa/<name>referenceIt happens next to a React element and resolves it through the dashboard's icon collection (
lookupWidgetIcon()from@jetpack-premium-analytics/icons).If the collection lacks a reference, it drops that icon and the row falls back to text segments instead of showing a blank one.
WIDGET_API_VERSIONmoves to 1.15.0A consumer may rely on the reference form. This is the client-side half of WOOA7S-2290.
With it, the
elementsof ajpa/toggle-groupattribute are serializable, which is what moving the attribute towidget.jsonneeds. #53778, which builds on this PR, moves the chart type attribute of the fifteen chart widgets there; the widgets keep their React element icons here.Related product discussion/links
jpa/<name>icon collection.Does this pull request change what data or activity we track or use?
No.
Testing instructions
Build the package with its dependencies
Open the Premium Analytics dashboard
Nothing changes: every chart widget still hands the control React elements, so the line/bar toggle in their headers draws as before.
Storybook:
Packages/Premium Analytics/Fields/ToggleGroupFieldrenders theDefaultandWithVisibleLabelstories as icon segments fromjpa/chart-lineandjpa/chart-barreferences.Running test
pnpm run testinprojects/packages/premium-analytics: the toggle group field suite covers the reference form and the text fallback.