Closed
Conversation
t-hamano
added
[Type] Performance
github-actions
bot
added
[Package] Core data
Mamaduka
reviewed
Feb 20, 2026
Comment on lines
177
to
190
|
|
||
| /** | ||
| * Returns an action object used to receive a single icon. | ||
| * | ||
| * @param {Object} icon Icon object (name, label, content). | ||
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function receiveIcon( icon ) { | ||
| return { | ||
| type: 'RECEIVE_ICON', | ||
| icon, | ||
| }; | ||
| } |
Member
There was a problem hiding this comment.
I think we can update the existing reducer and just reuse the receiveIcons action for both resolvers. That's what we're doing in other cases.
I'm also wondering whether the state should be keyed by a unique name, which might make it easier to work with. cc @mcsf
Contributor
Author
There was a problem hiding this comment.
Thanks for the feedback! I tried your suggestion in 9690621.
|
Size Change: +149 B (0%) Total Size: 6.84 MB
ℹ️ View Unchanged
|
t-hamano
requested review from
ajitbohra,
fabiankaegy and
nerrad
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Flaky tests detected in 9690621. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22214842255
|
Mamaduka
reviewed
Feb 20, 2026
Member
Mamaduka
left a comment
Mamaduka
left a comment
There was a problem hiding this comment.
Maybe we should just declare icons a new entity, so we don't have to reinvent the wheel. Technically, entities should support CRUD actions and aren't readonly endpoints, but there's already a case for __unstableBase, taxonomies and postTypes.
cc @youknowriad, @jsnajdr, what do you think?
| const { getIcon, getIcons } = unlock( select( coreDataStore ) ); | ||
| return { | ||
| selectedIcon: icon ? getIcon( icon ) : null, | ||
| allIcons: isInserterOpen ? getIcons() : [], |
Member
There was a problem hiding this comment.
The empty array fallback will trigger a warning.
| */ | ||
| export function getIcons( state: State ): Icon[] { | ||
| return state.icons ?? []; | ||
| return Object.values( state.icons ?? {} ); |
Member
There was a problem hiding this comment.
This would return a new array reference on each call.
Comment on lines
673
to
679
| case 'RECEIVE_ICONS': { | ||
| const next = { ...state }; | ||
| for ( const icon of action.icons ) { | ||
| next[ icon.name ] = icon; | ||
| } | ||
| return next; | ||
| } |
Member
There was a problem hiding this comment.
Maybe we should we return prev state early in action.icons is empty.
Contributor
Author
I remember that there was a small discussion about this previously. #72215 (comment) |
jsnajdr
reviewed
Feb 20, 2026
Member
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
icons should be an entity. We have a dedicated /wp/v2/icons endpoint to fetch them, and the generic entity code would do all the work for us.
The code in this PR is 99% OK but has some quirks caused by interaction between getIcon and getIcons. When I first fetch an individual icon, then there is an one-element array in state.icons. Then, when I call getIcons, the selector initially returns this one-element array and only then starts fetching the entire set. The UI will display an incomplete array of one or more individual icons that have been fetched so far. That's a potentially confusing result.
The reducer is also not able to account for deleted icons. It can only add new ones, but when something gets deleted, it won't remove anything from state.
Member
The main argument against an |
Contributor
Author
|
@jsnajdr Thanks for the feedback! My concern is that the icon endpoint is already shipped in WP 7.0, is it ok to create an icons entity now? |
Member
The The server endpoint itself should be quite independent from that, unless it's somehow utterly incompatible with the entity system. But that doesn't seem to be the case. The base URL is |
Member
|
Right similar ready only entities( icons, __unstableBase, taxonomies and postTypes) can't be edited or deleted. They just provide data from the server registry.
We could add a new @t-hamano, do you want me to also create a separate PR for icons as entities? |
Contributor
Author
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
3 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.
See #75715 (comment)
What?
The Icon Block loads all icons on initial render. This PR loads all icons the first time the icon picker is opened.
Why?
Performance improvement
How?
Create a new private selector to get a single icon by name.
Testing Instructions
/wp-json/wp/v2/icon.Screenshots or screencast
cf95ce8b25ce455647511adeb1a47ed1.mp4