Repository navigation
Icons: ship required icons for admin bar and menu with public: false - #79451
Conversation
|
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. |
|
Thank you for your PR. However, it is difficult to move forward with this PR at this time.
It seems we need a flag to decide whether to expose icons in the Icon block, regardless of whether we move forward with #79102. |
|
Flaky tests detected in 8cf726d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/28036189826
|
|
Commented on the related PRs. I felt like we had an agreement about a |
|
The issue is that when an icon is published, it simultaneously becomes selectable within the icon block. For instance, few users would want to use icons like 'dashboard' or 'grid' in their post content. I feel there is a need for a way to register icons in the icon registry without publishing them in the icon block, as I would like to propose here. |
Just to be sure I understand: are you saying |
I see—so, this is what you mean? I believe this approach will allow us to achieve our goals without significantly altering the synchronization method between the core and Gutenberg. [
// Not shipped to the core
{
"slug": "one",
"label": "One",
"filePath": "library/one.svg"
},
// Published in the Icon block
{
"slug": "two",
"label": "Two",
"filePath": "library/two.svg",
"public": true
},
// Not published in the icon block
{
"slug": "three",
"label": "Three",
"filePath": "library/three.svg",
"public": false
}
] |
|
This sesms reasonable at first glance, but @mcsf had some thoughts previously, so I'd love to pick his brain here |
|
This seems reasonable to me too. :) |
8cf726d to
6197662
Compare
|
I'm implementing the suggestion as discussed above. Marking this PR as draft and will re-request for review when it's ready 🙂 |
🤖 PR meta 🤖📦 Bundle sizeSize Change: 0 B Total Size: 8 MB From ⚡ PerformanceShow the resultsClient side metrics exclude the server response time. front-end-block-theme
front-end-classic-theme
media-processing
media-upload
post-editor
site-editor
|
8c3aebc to
dc2e30a
Compare
6197662 to
c51d56a
Compare
402e768 to
a087ca3
Compare
c51d56a to
4bef6f4
Compare
|
Sorry for the mass ping everyone... bad |
| if ( has_action( 'admin_bar_menu', 'wp_admin_bar_command_palette_menu' ) ) { | ||
| remove_action( 'admin_bar_menu', 'wp_admin_bar_command_palette_menu', 55 ); | ||
| } | ||
| add_action( 'admin_bar_menu', 'gutenberg_admin_bar_command_palette_menu', 55 ); |
There was a problem hiding this comment.
Is this change intentional?
There was a problem hiding this comment.
Oh i get it now. I think this probably should have its own PR.
There was a problem hiding this comment.
Actually the only required one is this move: https://github.com/WordPress/gutenberg/pull/79451/changes#r3970437640
I also moved this one inside the if ( ! function_exists( 'wp_admin_bar_command_palette_menu' ) ) { check, to make it consistent. Also, I think it's more correct? We want to add the node and enqueue the style from this GB compat only when Core still does not support command palette (version < 7.0). I hope I'm not missing anything 😬
I tested WP 6.9 and 7.0 with this GB PR via Playground and things seem to be still working.
There was a problem hiding this comment.
I think this probably should have its own PR.
I thought that, too, but I wanted to test everything with a single Playground link to make sure everything works well together 😄 I can take this one out and create another PR later when we're ready to merge things.
Marks the icons that WordPress/wordpress-develop#12270 adds to Core's icon library as `"public": false`: they are shipped and registered for server-side use, but stay out of the icons REST API and so out of the Icon block. brush, dashboard, link, media, page, pin, plugins, post, sites, tool, update `wordpress` is the twelfth icon that Core PR needs. It is already flagged by #82634, which introduces the non-public state and uses it as its first consumer, so it is not repeated here. `false` rather than `true` because these are admin chrome, not post content. Promoting an icon later is trivial; demoting one is a breaking change.
Part of #82723
Related to: WordPress/wordpress-develop#12270
What?
Make the following icons available to
wp_get_icon()by addingpublic: falseto the manifest.brushdashboardlinkmediapagepinpluginssitestoolupdateWhy?
The above icons are needed as the replacement for the old dashicons in the admin UI.
Testing Instructions
It's way easier to test with WordPress/wordpress-develop#12270. Open the following Playground link, which test both that Core PR and this Gutenberg PR:
https://playground.wordpress.net/?core-pr=12270&gutenberg-pr=79451
Then:
core/wordpress):Use of AI Tools
I used Claude Opus to generate the changes, then I also double checked it.