Repository navigation
Conversation
|
Hi there! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
ec0faf9 to
6ec651a
Compare
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @lucasmendes-design. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
cc-ing (maybe) interested parties :) @t-hamano @jasmussen @tyxla @scruffian @lucasmendes-design |
|
These all appear to be solid like-for-like replacements, though I feel least supportive of the |
|
@jeffpaul Thanks for checking!
Yep, as noted in the PR description, I'm not sure about this as well.
You mean 
But my worry is that the icon slug is not related to "site" at all. I would expect there is something like Another idea is to use this icon, to represent a "site switcher": 
|
|
A few things from my end as I'm looking at this work:
|
|
@fushar yes the |
Thanks for the suggestion. I spent a good chunk of my day today trying to make this as a Gutenberg experiment. With the help of Claude, I (we) came up with this PR: WordPress/gutenberg#79588. Now, I understand that the changes are complicated, but I can't simplify it any further. Reasons:
Am I approaching this correctly? Or do you have better idea for a cleaner approach? Thanks! cc: @mcsf |
|
It looks great ✨ I noticed a few things that I'm not fully sure belong to the PR scope. Icon sizeIn most cases, the icon size is 22px, but the W symbol is 24px.
Not sure if this is intentional or not. Color on the frontendOn the frontend, the color at "Sites" iconRegarding the icon for Sites, I agree that I've been testing it on Arc, Chrome, and Safari and on multiple viewports. And they all look good ⭐ |
Hmm, this seems correct to me. There are side ways for achieving some of the things you mentioned, but not for others, unless we want to be super hacky. A bummer that this part of the menu is not as extensible as usual. Thank you for trying, though. Another way to get more attention on this would be to post a call for testing on the https://make.wordpress.org/core blog. |
I'd say so, at least for the purposes of a Gutenberg experiment. I left a comment or two in that PR. |
@jeffpaul: I agree that it looks better than a 2x2 grid. For the purpose of demonstration, I updated this PR to use that for the time being: 
|
|
@fcoveram: Thanks for the thorough testing!
Yeah, it is intentional. I'm not sure if it's the best way though. It's to achieve the same look and feel with the existing W dashicon. See the above image comparison, pasted here again: 
Right now, if we use 22px, then the W logo is too small, because there's outer gap in the SVG. Maybe we need to fill the entire SVG box without any outer gap.
Yeah agree that 0.85 looks better. But I'm not sure if we can just update it only for the SVG icon. It's part of the Fresh color scheme; maybe we should do a separate PR to update the scheme 😄 |
|
All right everyone, thanks for taking a look. I want to summarize where we're at with this PR:
It feels like there's interest to push at least some parts of it in 7.1, and we're not that sure if we can achieve everything by 7.1. @t-hamano, what's your opinion on this? It seems you have the most context for all this icon API discussion 😄 cc: @tyxla, @mcsf, @scruffian. |
Leveraging the icon API for the dashboard icons is a great idea, and introducing a add_action( 'init', function () {
wp_unregister_icon_collection( 'core' );
}, 20 );add_action( 'init', function () {
wp_unregister_icon( 'core/info' );
}, 20 );In this case, what happens to the core menu icons registered by the icon API? This is my biggest concern. Here are the ideas I can come up with at the moment:
Do you have any other ideas? |
|
Thanks for taking a look.
Hmm interesting question. I would say, the last option (consider Otherwise, we will also need another way to get the SVG string for the admin bar icons as well, because in this PR I also use |
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
This reverts commit 0ba603a.
7598ddf to
15cb2ce
Compare
|
@fushar, my name was unlinked could you please check |
Test ReportEnvironment
ResultsCore icons
Back-compat with plugin icons
Color schemes
Notes
|
| return is_string( $icon_name ) | ||
| && 1 === preg_match( '#^[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?/[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?$#D', $icon_name ); |
There was a problem hiding this comment.
Nit: && already casts the result to bool, so 1 === isn't needed.
| return is_string( $icon_name ) | |
| && 1 === preg_match( '#^[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?/[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?$#D', $icon_name ); | |
| return is_string( $icon_name ) | |
| && preg_match( '#^[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?/[a-z0-9](?:[a-z0-9_-]*[a-z0-9])?$#D', $icon_name ); |
| array( | ||
| 'id' => 'my-account', | ||
| 'parent' => 'top-secondary', | ||
| 'icon' => empty( $avatar ) ? 'core-admin/people' : '', |
There was a problem hiding this comment.
Nit: false would match the default value of icon in add_node().
| 'icon' => empty( $avatar ) ? 'core-admin/people' : '', | |
| 'icon' => empty( $avatar ) ? 'core-admin/people' : false, |
| * 6: Icon for top level menu. | ||
| * | ||
| * Dashicons back-compat: the Dashicons values of the icons are kept. They are | ||
| * replaced with SVG icons at render time, see _wp_replace_menu_dashicon(). | ||
| * |
There was a problem hiding this comment.
Nit: This only concerns the icon element, so how about folding it into item 6?
| * 6: Icon for top level menu. | |
| * | |
| * Dashicons back-compat: the Dashicons values of the icons are kept. They are | |
| * replaced with SVG icons at render time, see _wp_replace_menu_dashicon(). | |
| * | |
| * 6: Icon for top level menu. Dashicons values are kept here and replaced | |
| * with SVG icons at render time, see _wp_replace_menu_dashicon(). | |
| * |
| * @param string $dashicon The Dashicon class name, e.g. 'dashicons-admin-post'. | ||
| * @return string Namespaced icon name of the replacement, or the given value if it has no replacement. | ||
| */ | ||
| function _wp_replace_menu_dashicon( $dashicon ) { |
There was a problem hiding this comment.
This is just an array lookup, which feels too simple to warrant its own function. Since it's only used in _wp_menu_output(), how about inlining the map there, defined once before the loop?
| * @param string $color_scheme The name of the color scheme. | ||
| * @return string The CSS, or an empty string if the scheme needs none. | ||
| */ | ||
| function _wp_get_color_scheme_menu_icon_styles( $color_scheme ) { |
There was a problem hiding this comment.
This feels a bit roundabout. What about the custom property approach from #14011? A rough sketch:
admin-menu.css reads the colors, falling back to the current defaults:
#adminmenu div.wp-menu-image {
color: var(--wp-admin--menu--icon, #a7aaad);
}
#adminmenu li:hover div.wp-menu-image /* , ... */ {
color: var(--wp-admin--menu--icon-focus, #72aee6);
}
#adminmenu li.current div.wp-menu-image /* , ... */ {
color: var(--wp-admin--menu--icon-current, #fff);
}Core schemes set the properties in _admin.scss:
#adminmenu {
--wp-admin--menu--icon: #{variables.$menu-icon};
--wp-admin--menu--icon-focus: #{variables.$menu-highlight-icon};
--wp-admin--menu--icon-current: #{variables.$menu-current-icon};
}Other schemes get them from icon_colors:
$colors = $_wp_admin_css_colors[ $color_scheme ]->icon_colors;
wp_add_inline_style(
'admin-menu',
sprintf(
'#adminmenu{--wp-admin--menu--icon:%s;--wp-admin--menu--icon-focus:%s;--wp-admin--menu--icon-current:%s}',
sanitize_hex_color( $colors['base'] ),
sanitize_hex_color( $colors['focus'] ),
sanitize_hex_color( $colors['current'] )
)
);On live preview, user-profile.js sets them from the icon_colors JSON the picker already has:
const { icons } = JSON.parse( $this.children( '.icon_colors' ).val() );
const menu = document.getElementById( 'adminmenu' );
menu.style.setProperty( '--wp-admin--menu--icon', icons.base );
menu.style.setProperty( '--wp-admin--menu--icon-focus', icons.focus );
menu.style.setProperty( '--wp-admin--menu--icon-current', icons.current );The property names would become de facto public, but #14011 has the same trade-off, so it might be good to decide on the naming for both PRs together.
13 participants
Trac ticket: https://core.trac.wordpress.org/ticket/65089
This PR replaces the dashicons usage in admin bar and menu (sidebar) with
@wordpress/icons, via the newly-introducedwp_get_icon()function, which outputs an inline SVG to be rendered.New icon mapping
Admin bar
dashicons-wordpresscore-admin/wordpressdashicons-menu-altcore-admin/menudashicons-admin-multisitecore-admin/sitesdashicons-dashboardcore-admin/dashboarddashicons-admin-homecore-admin/homeCustomize
dashicons-admin-appearancedashicons-admin-customizercore-admin/brushdashicons-editcore-admin/pencildashicons-pluscore-admin/plusdashicons-admin-commentscore-admin/commentdashicons-updatecore-admin/updateCommand Palette
dashicons-searchcore-admin/searchdashicons-admin-userscore-admin/peopleAdmin menu (left sidebar)
For simplicity, only showing new items not present in the admin bar above.
dashicons-admin-postcore-admin/pindashicons-admin-mediacore-admin/mediadashicons-admin-linkscore-admin/linkdashicons-admin-pagecore-admin/pagedashicons-admin-pluginscore-admin/pluginsdashicons-admin-toolscore-admin/tooldashicons-admin-settingscore-admin/settingsdashicons-admin-collapsecore-admin/chevron-leftOpen questions
I'm not sure aboutMy Sitesicon. The closest thing that I think fits is thecore/gridicon (see screenshot below). Open to alternatives.Updated toUpdated tocore/block-metafor the time being, still open to alternatives.core/sitesfrom Icons: Add "sites" icon. gutenberg#80094.Screenshots
WP Admin
WP Admin - Multisite
Frontend
Before
After
Mobile
Before
After
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 4.8
Used for: code generation, with my supervision.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.