Admin bar: Replace home/odometer dashicon with actual site icon if set#11781
Admin bar: Replace home/odometer dashicon with actual site icon if set#11781fushar wants to merge 9 commits into
Conversation
|
Hi @fushar! 👋 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. |
|
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. |
fushar
force-pushed
the
admin-bar-site-icon
branch
2 times, most recently
from
fd1099a to
646995d
Compare
fushar
changed the title
fushar
changed the title
fushar
force-pushed
the
admin-bar-site-icon
branch
from
646995d to
4c0c32f
Compare
|
Thanks for the PR! I'm less sure about changing the dashboard (odometer) icon, as it's specifically used to get to the dashboard, and it might be confusing if both icons look the same but lead to different places depending on the context. It also seems that the ticket only suggested changing the home icon. |
|
Trac comment: https://core.trac.wordpress.org/ticket/65088#comment:6 The thing to notice is that this isn't a single link, it's a contextual entry point. Depending on where you are, the same node can mean "Visit Site" or open a dropdown to the Dashboard, Plugins, and Themes. So the real question isn't "which destination should the icon show," it's "what does this touchpoint represent." A destination icon over-promises. The odometer says "this goes to the dashboard," and a house says "this goes home," but the node actually does several site-level things depending on context. That mismatch is the real source of confusion, the icon makes a promise the node doesn't always keep. The site icon removes that. The one constant across every context here is the site itself. everything in this node is site-scoped. So "site icon + site name" sets the right expectation, "things you can do for this site", and the dropdown shows the specifics. Let me know your thoughts @SergeyBiryukov |
To me, the current behavior is actually more confusing: the icon changes depending on whether we're on wp-admin or site frontend. It's not immediately clear that the odometer icon actually signifies "the wp-admin Dashboard". When we update the icon to be the site icon in both cases, it becomes a consistent, fixed-point context of "the current site". It provides an entry for site-specific dropdown actions such as going to wp-admin and going to the site frontend. It when clicked having different destination is just a byproduct of the different first action in the dropdown. What do you think? |
fushar
changed the title
|
I agree with this very much. The icon changing, but the label not, makes no sense to me, and it feels reasonable as a bandaid to at least unify them. But I would go a step further to suggest that the odometer changing like that reveals a fundamental underlying problem with how it's worked so far. To that end, I support what Lucas is proposing. A more bold alternative is to recast the WordPress icon to always take you to the dashboard (to your WordPress) and the site title always taking you to the frontend, which is what the topbar test plugin linked here does, if you'd like to try it. Key with showing a site logo is also to ensure that, at a glance, you can always see which WordPress site you're on. If you are a power-user with multiple open WordPress tabs, unless you're careful, you might end up uploading private information to the wrong website. So we need clear delineation between sites, which the distinct-per-site site log would help do. I've seen this happen in support issues. |
Let's proceed with this, and then we can circle back on different actions for the W logo and Site icon + title. I'm assuming there are some plugins that can use the site name dropdown as well, so I think we need to investigate that a bit more. |
| width: 20px; | ||
| height: 20px; | ||
| margin: 0 6px 0 0; | ||
| vertical-align: -5px; |
There was a problem hiding this comment.
Would be nice to document where the magical -5px value comes from.
There was a problem hiding this comment.
After some exploration, I decided to update the logic (at e6dece6) to follow how my-account node handles avatar image centering (see below). Still some magic number, but it can be seen as adjusting the margin, to offset the text baseline. I think this kind of margin-based adjustment is common in Core's CSS.
wordpress-develop/src/wp-includes/css/admin-bar.css
Lines 489 to 490 in e904e28
| 'menu_title' => $title, | ||
| ); | ||
|
|
||
| $site_icon_url = get_site_icon_url( 64 ); |
There was a problem hiding this comment.
Existing multisite toolbar icons are gated by wp_admin_bar_show_site_icons, documented as the recommended way to hide toolbar site icons. After the change, here plugins that disable toolbar site icons still get one in the primary site-name item. Maybe we should gate this with the same filter and add a test for the false case?
| ); | ||
|
|
||
| $site_icon_url = get_site_icon_url( 64 ); | ||
| if ( $site_icon_url ) { |
There was a problem hiding this comment.
Also see the is_network_admin() and is_user_admin() checks above. When those are true, we're not really working with the "current site" so prepending the current blog icon may not be relevant. Do we need to handle those cases separately? Have we done any multisite testing of these changes?
There was a problem hiding this comment.
Thanks, I totally forgot about multisite. I also realized that the My Sites dropdown already renders the site icon, so this PR makes it even more consistent 😄
I added checks for is_network_admin() and is_user_admin() in 0975127. Now the icon does not render on those pages.
fushar
force-pushed
the
admin-bar-site-icon
branch
from
277561c to
e6dece6
Compare
| /** This filter is documented in wp-includes/admin-bar.php */ | ||
| $show_site_icons = apply_filters( 'wp_admin_bar_show_site_icons', true ); | ||
|
|
||
| if ( $show_site_icons ) { |
There was a problem hiding this comment.
I wonder if we can fold this if in the parent if()? I'm thinking it will make the code more readable due to one fewer level.
| if ( $show_site_icons ) { | ||
| $site_icon_url = get_site_icon_url( 64 ); | ||
| if ( $site_icon_url ) { | ||
| $title = '<img class="site-icon" src="' . esc_url( $site_icon_url ) . '" alt="" />' . $title; |
There was a problem hiding this comment.
Does it make sense to provide width / height attributes? That's what we're doing for multisite:
|
|
||
| if ( $show_site_icons ) { | ||
| $site_icon_url = get_site_icon_url( 64 ); | ||
| if ( $site_icon_url ) { |
There was a problem hiding this comment.
Looks like we're using has_site_icon() for such a check in another place. Should we use that? We could also fold that at the top if level and make the logic even simpler that way.
There was a problem hiding this comment.
TIL about that. WP has so many hidden functions haha. OK, updated here and the if levels: 2b4e5d8
| $show_site_icons = apply_filters( 'wp_admin_bar_show_site_icons', true ); | ||
|
|
||
| if ( $show_site_icons ) { | ||
| $site_icon_url = get_site_icon_url( 64 ); |
There was a problem hiding this comment.
Where does the 64 come from? I believe in core we use 16 or 32, so should we go with that instead?
There was a problem hiding this comment.
OK this is a bit complicated. In mobile, the size is 28, so for retina we need at least 56. I thought get_site_icon_url() only supports powers of 2, so naively I used 64 here 😅
But then you showed me how multisite renders the blavatar, which allows specifying non-retina and retina resolutions, so I took that as inspiration. I updated the sizes to be 28 and 56 (on 7624e1b), which, according to the logic in src/wp-admin/includes/class-wp-site-icon.php, will resolve to 32 and 150. The sizes are from a fixed set.
So:
- Desktop: the size is 20, browser will download 32 (still >= 20), retina will download 150 (still >= 20x2)
- Mobile: the size is 28, browser will download 32 (still >= 28), retina will download 150 (still >= 28x2)
Does that make sense or did I explain that poorly 😄
There was a problem hiding this comment.
Makes sense, thanks for elaborating!
But the inconsistency with what's already in other places in core bothers me a bit.
I'm actually fine with going with what's in core already, and then updating these instances and the ones in core in a follow-up, so they remain consistent, but use a larger size.
WDYT?
There was a problem hiding this comment.
I'm actually fine with going with what's in core already
Sorry I'm a bit lost, what are you suggesting us to go with? 32 only without 2x for retina? Or 16 and 32 (but 16 is already too small for a 20x20 icon).
There was a problem hiding this comment.
I guess I'm ruminating on reusing what we have and staying consistent at the same time.
I don't mean to block you. Feel free to go with what you're suggesting here, but let's add a TODO comment to ensure we follow-up and be consistent across core later.
There was a problem hiding this comment.
I guess I don't understand what "reusing" means here? Both 28 and 32 will resolve to the same 32px image URL.
Or you mean reuse the constants, so we use 32 and 64, like this PR #11601 ?
There was a problem hiding this comment.
^ I did that anyway. It looks and feels better now: 88aef4e
| margin: 0 8px 2px -2px; | ||
| } | ||
|
|
||
| #wpadminbar .quicklinks li img.blavatar { |
There was a problem hiding this comment.
Won't this also round the blavatars in the "My Sites" menu? Is that intended and has it been discussed?
There was a problem hiding this comment.
It's intentional (from my side) but we never actually discussed it because we just discovered that case now 😄
I think it's fine and consistent though. Even before this change, if you go to Settings -> General, and upload a site icon, somehow we already round it in the preview:
cc-ing: @jasmussen and @lucasmendes-design just in case.
There was a problem hiding this comment.
Also when cropping the image from media library; here it says "As an app icon and browser icon", already suggesting the in-app icon is rounded and browser icon (favicon) is not:
There was a problem hiding this comment.
In the browser, the icon has rounded corners only if you upload an image with that. Otherwise, it's like what Ashar described. LGTM!
There was a problem hiding this comment.
This is one of the strongest reasons for why I firmly stand by my proposal that the user avatar should also be circular. We are working with images that are 20x20px in size, people of low vision need more than the contents inside for them to indicate concepts. If we establish that sites are round-rects, and users are circles, we provide a clean and clear silhouette that is immediately accessible way to differentiating between the two.
| height: 20px; | ||
| vertical-align: middle; | ||
| margin: -2px 6px 0 0; | ||
| background: #f0f0f1; |
There was a problem hiding this comment.
Do we have a way to deal with this for transparent site icons throughout the different color schemes?
There was a problem hiding this comment.
So I explored various colors; I think this is good enough for all color schemes for transparent icons. This is because the text color is white as well. EXCEPT for Light scheme; it's not different enough from the admin bar background color. But I don't think any other darker color will work as it might break the intended transparent image. So I believe this is the best choice for this purpose.
There was a problem hiding this comment.
Thanks. It could use a comment on why we chose this color - it would be helpful if we choose to update it or just wonder why this one was selected in the first place.
There was a problem hiding this comment.
I actually chose #f0f0f1 because it's used in so many places already in the CSS files as background, like body. But now I checked it turns out it's the same color used in the user avatar node 😄, which makes even more sense, so I added a comment reflecting that. d803c05
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Pending one minor text fix, this is looking good to me.
Let's also make sure that we have a design approval here before we 🚢 .
| $node_site_name = $wp_admin_bar->get_node( 'site-name' ); | ||
|
|
||
| $this->assertStringContainsString( '<img class="site-icon"', $node_site_name->title ); | ||
| $this->assertStringContainsString( esc_url( get_site_icon_url( 28 ) ), $node_site_name->title ); |
There was a problem hiding this comment.
Seems like we need to fix this size as well.
| $this->assertStringContainsString( esc_url( get_site_icon_url( 28 ) ), $node_site_name->title ); | |
| $this->assertStringContainsString( esc_url( get_site_icon_url( 32 ) ), $node_site_name->title ); |
|
@tyxla Thanks for the super detailed review. I really appreciate it.
I believe @lucasmendes-design & @jasmussen have already given their blessings, can you confirm 😄 |
No worries, let's just also make sure all checks pass - there seem to be some failures right now (seem to be one-off infra failures at first glance) |
I often have such failures in my other PR(s). I'm pretty sure they are just flaky runs, but I don't have permission to rerun the jobs 😬 usually I just force-push empty commit just to rerun them 😞 not sure what's the best practices here. |
Got it. Yeah, force-pushing is the easiest thing to do if you can't rerun them. I've re-ran them for you now. |
5 participants
Trac ticket: https://core.trac.wordpress.org/ticket/65088
This PR replaces the home / odometer dashicon, with the actual site icon, if set, in the admin bar. The icon will be 20x20px, with border-radius of 2px and background-color of
#f0f0f1.If the site has not set site icon, then the default dashicons (home / odometer) is NOT changed.
Screenshot
Before
After
Before (mobile)
After (mobile)
Use of AI Tools
I used Codex with GPT-5.5 to help with the CSS implementation, 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.