Make WordPress Core

Opened 5 years ago

Closed 3 years ago

Last modified 3 years ago

#46657 closed enhancement (fixed)

Show site icon in My Sites admin menu

Reported by: rmccue's profile rmccue Owned by: audrasjb's profile audrasjb
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Networks and Sites Keywords: has-patch has-screenshots needs-testing
Focuses: multisite Cc:

Description

The My Sites menu shows a WordPress logo next to every site. This isn't super useful, as you end up with a list full of WordPress icons.

This feature appears to have been modelled originally on WordPress.com's "blavatar" feature (and the class is named thus as well). However on WordPress.com, the site's icon is used instead of a generic WordPress icon.

With site icons in WordPress core, we should use these instead.

Attachments (7)

46657.diff (1.2 KB) - added by rmccue 5 years ago.
Initial proof-of-concept
46657.2.diff (1.5 KB) - added by rmccue 5 years ago.
Add filter to allow custom site icons
46657.3.diff (1.5 KB) - added by rmccue 5 years ago.
Escape URL before output
46657.4.diff (1.6 KB) - added by rmccue 5 years ago.
Add alt and update CSS
46657.5.diff (1.3 KB) - added by audrasjb 3 years ago.
patch refreshed against trunk
Capture d’écran 2021-05-06 à 00.09.48.png (69.7 KB) - added by audrasjb 3 years ago.
46657.5.diff works fine
46657.6.diff (1.3 KB) - added by desrosj 3 years ago.

Download all attachments as: .zip

Change History (21)

@rmccue
5 years ago

Initial proof-of-concept

#1 @rmccue
5 years ago

  • Keywords has-patch added
  • Version set to 4.3

@rmccue
5 years ago

Add filter to allow custom site icons

#2 @rmccue
5 years ago

46657.2.diff adds the site icon in place of the default WordPress logo. This replaces the <div class="blavatar"></div> with <img src="..." class="blavatar" /> if the site has an icon.

For example, with a site with an icon and one without:

https://i.imgur.com/bumICpf.png

#3 @desrosj
5 years ago

Related: #33573.

#4 @joemcgill
5 years ago

@rmccue I'd suggest using esc_url() here. It's best practice—particularly since the output of that function is filterable so the value can't be trusted—and would be consistent with other places in core where get_site_icon_url() is used.

See examples:

@rmccue
5 years ago

Escape URL before output

#5 @rmccue
5 years ago

46657.3.diff uses esc_url, and also uses string concatenation to better match core's general style.

#6 @afercia
5 years ago

Cool.

For better accessibility, the image needs an empty alt="" attribute, otherwise screen readers will announce the image filename.

CSS:

  • the image inherits float: left so display: inline-block doesn't do anything
  • .blavatar:before targets also the image: I'd suggest to add a rule to target img.blavatar:before and reset the content property to content: none

@rmccue
5 years ago

Add alt and update CSS

#7 @rmccue
5 years ago

Thanks @afercia!

46657.4.diff adds the alt and reworks the CSS to keep it DRY and avoid a (nonsense) img:before rule.

#8 @desrosj
5 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to Future Release

#9 @Hareesh Pillai
3 years ago

  • Keywords needs-refresh added; has-patch needs-testing removed
  • Milestone changed from Future Release to 5.8

This is a nice improvement to have. Moving it to the next milestone.
Also, I'm unable to apply the patch, it needs refresh.

@audrasjb
3 years ago

patch refreshed against trunk

@audrasjb
3 years ago

46657.5.diff works fine

#10 @audrasjb
3 years ago

  • Component changed from Administration to Networks and Sites
  • Keywords has-patch has-screenshots added; needs-refresh removed
  • Owner set to audrasjb
  • Status changed from new to accepted
  • Version 4.3 deleted

Thanks for moving this to 5.8!
I refreshed the patch and it works really fine on my side.

I think it's ready to go this time!

#11 @audrasjb
3 years ago

  • Focuses css added

@desrosj
3 years ago

#12 @desrosj
3 years ago

  • Focuses css removed
  • Keywords needs-testing added

Was reviewing this one and made a few modifications before attaching 46657.6.diff.

  1. The patch now makes use of the has_site_icon() function instead of calling get_site_icon_url() directly and needing to store the result in a variable.
  2. Switches to using sprintf() to populate the image tag.
  3. Added a srcset attribute in order to support specifying a 2x image. This allows the browser to choose the desired image based on the user's pixel density. This pattern is also used for embed titles for oEmbeds.

Please test!

#13 @audrasjb
3 years ago

Those are great points, thanks for the new patch.
It's good to go on my side!

#14 @desrosj
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

This was fixed in [50834], but I missed the # preceding the ticket number.

Last edited 3 years ago by desrosj (previous) (diff)
Note: See TracTickets for help on using tickets.