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 |
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
|
| * @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.
There was a problem hiding this comment.
Yeah, the code is not that great. But I agree with @aduth's comment here: #12270 (comment)
With that in mind, I'd lean toward the inline approach this pull request uses, rather than new custom properties like explored elsewhere in #12270 (comment) and in #14011, so that once tokens are available to use across wp-admin, admin-menu.css can use them directly and the behavior in _wp_get_color_scheme_menu_icon_styles can apply for color schemes not based on design tokens (which, as a future state, we'd hope would be increasingly uncommon). This also helps avoid an extra public API to maintain.
|
Thanks @t-hamano for reviewing and resolving GitHub comments, I'll continue tomorrow! |
|
Chiming in with a design review. Feel free to dismiss these thoughts if this isn't timely. Stroke width on mobile With the stroke scaling work (Gutenberg#84081), the 32px size becomes bolder. I see that WordPress icon scale The WordPress icon looks slightly bigger because it uses 20 of the 24 units in its container. Until the icon becomes stroke-based, perhaps we can scale it to 18px inside the same 24px container. This also relates to Gutenberg#84105. Search icon orientation The new search icon faces the opposite way, and |
But isn't this also the case with Dashicons before this PR? I thought that's intended 😄 the W's circle before and after this PR has the same size I believe. |
I don't know what structure Dashicons followed, and the current SVG exceeds the grid. It is definitely a design problem, but the solution is tricky, as logos scale proportionally and the ring that wraps the W symbol does as well. |
Hmm, what about the frontend search icon (the one in top-right), do we want to flip that as well? Also, wouldn't it be much easier and consistent if we flip the one in the command palette modal instead? Then everything will match new new search icon? |
I'd defer this to @jasmussen because I read the idea is to NOT make |
|
I made a few iterations to the draft dev note. I believe it's now in a good shape. I ran this with my Opus 5.5 agent (which had access to a working local WordPress dev server) and it verified the claims 🙂 The next step would be to agree on the direction of the back-compat strategy mentioned in the dev note. |
I don't see a reason as the input visible after clicking on the icon shows the same orientation.
Not sure if it's easier, but coherent for sure. I'm not aware of the timelines and plans for the CP and whether this change is doable before the deadline. So pinging @senadir as his input is relevant here.
Joen's words make sense. Then I wonder if we can approach it differently and apply an ad hoc property that scales it to 18px inside the same 24px container. |
|
The orientation of the search icon has been discussed a few times before. See WordPress/gutenberg#72070. My understanding is that in LTR languages, the search icon should face left. So the search icon in the Command Palette should be facing the correct direction: 
I think the ones that need fixing are SearchableSelectControl, the admin bar in the admin, and the admin bar on the front end.
cc @WordPress/gutenberg-components |
|
Thanks for the additional context 😄 the search icon direction is now fixed in the latest commit 6c3fc8b. |
|
The icon direction looks correct now ✨ |
14 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.