WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 4 weeks ago

#31352 accepted defect (bug)

Media icons are not retina friendly

Reported by: iseulde Owned by: joemcgill
Milestone: Future Release Priority: normal
Severity: normal Version: 3.9
Component: Media Keywords: has-patch has-ui-feedback needs-testing dev-feedback needs-design
Focuses: ui, administration Cc:

Description

See screenshot and [27726].

Attachments (9)

Screen Shot 2015-02-16 at 20.20.08.png (9.9 KB) - added by iseulde 3 years ago.
dashicons-media-2x.zip (16.3 KB) - added by melchoyce 3 years ago.
dashicon-media-2x-optimized.zip (3.6 KB) - added by joemcgill 3 years ago.
Optimized dashicon .png files.
31352.diff (4.0 KB) - added by joemcgill 3 years ago.
dashicons-media-svgs.zip (12.7 KB) - added by melchoyce 3 years ago.
media-files-40px-optimized.zip (7.9 KB) - added by joemcgill 3 years ago.
New media files optimized
31352.2.diff (6.1 KB) - added by joemcgill 3 years ago.
31352.3.diff (9.7 KB) - added by joemcgill 3 years ago.
31352.4.diff (7.6 KB) - added by joemcgill 3 years ago.
Replaces bad diff in 31352.3.diff

Download all attachments as: .zip

Change History (36)

#1 follow-up: @wonderboymusic
3 years ago

  • Milestone changed from Future Release to 4.2

@melchoyce - can we get your comments here, please?

#2 in reply to: ↑ 1 @melchoyce
3 years ago

Replying to wonderboymusic:

@melchoyce - can we get your comments here, please?

Sorry, what's the question? Double checked and yeah, seeing 1x images instead of 2x. I can re-attach the 2x ones here, but we should really try to figure out a way to use something vector here, either the dashicons font directly or svgs.

#3 @wonderboymusic
3 years ago

@melchoyce I will defer to you on the best path forward - I need to go to retina camp and remind myself how these work

#4 follow-up: @melchoyce
3 years ago

Do you remember why we couldn't use dashicons in the first place? I remember there was a good reason, but I don't actually remember what that was anymore. Do you know if it's still an issue?

#5 in reply to: ↑ 4 ; follow-up: @ocean90
3 years ago

Replying to melchoyce:

Do you remember why we couldn't use dashicons in the first place?

See #26650 and [27726]. The issue is that wp_get_attachment_image_src() can only return images.

#6 in reply to: ↑ 5 @melchoyce
3 years ago

Replying to ocean90:

Replying to melchoyce:

Do you remember why we couldn't use dashicons in the first place?

See #26650 and [27726]. The issue is that wp_get_attachment_image_src() can only return images.

Do you know if it considers svgs images?

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

@joemcgill
3 years ago

Optimized dashicon .png files.

@joemcgill
3 years ago

#8 @joemcgill
3 years ago

  • Focuses ui administration added
  • Keywords has-patch ui-feedback needs-testing dev-feedback added; needs-patch removed

31352.diff gets the ball rolling on this by adding @melchoyce's dashicon files (optimized in dashicon-media-2x-optimized.zip) to the /wp-includes/images/media/ directory and referencing them in srcset attributes in media templates by adding an option to views.js when an icon is present.

Concerns:

  • At the moment, the new icon files do not match the 1x versions already in use: https://cloudup.com/cAs9F7XWMAQ. We either need to update both or change the new 2x files to relate to the ones already in core. I would defer to Mel here.
  • Using srcset with x descriptors has pretty good, but not complete browser support: http://caniuse.com/#feat=srcset. If we wanted to provide support for all browsers, we would need to include a polyfill. Not sure we want to add a js dependency here, so I opted not to.
  • There's probably a more robust way to take the name of the 1x icon to create the 2x file name than what I've done in the inital patch, but it's a decent proof of concept.

Alternate solutions considered:

  • Simply replace the current icon files with the 2x versions and serve oversized images to those who don't need them. Not ideal, but less markup. I wouldn't recommend this, but it's an option.
  • Using SVGs. I couldn't find any prior art in the admin for using inline SVGs in the UI. There are several places where we use SVGs as background images via CSS, but not inline. Ultimately, this would be an ideal case for using SVGs but there have been concerns expressed with doing so.

#9 @melchoyce
3 years ago

Attached the icons as svgs. Let's use these.

This ticket was mentioned in Slack in #design-dashicons by melchoyce. View the logs.


3 years ago

#11 @joemcgill
3 years ago

There is a desire to update dashicons so that they could be used as inline SVGs instead as background images via CSS. Once the dashicon SVGs are updated (see previously linked Slack log). If we want to wait for an SVG solution, those updates would need to happen before this issue could be resolved.

Otherwise, we can add 2x png files for now and look at updating them to SVG once the dashicon improvements are merged.

This ticket was mentioned in Slack in #design-dashicons by ryelle. View the logs.


3 years ago

@joemcgill
3 years ago

New media files optimized

@joemcgill
3 years ago

#14 @joemcgill
3 years ago

I've compressed the dashicon files from @melchoyce and colored them to match what was already there (hex: 888888) in media-files-40px-optimized.zip. These files will need to overwrite (and add to) the existing files in src/wp-includes/images/media/ as shown in 31352.2.diff.

By the way, If there's a better way to deal with media files in diffs, let me know.

@joemcgill
3 years ago

@joemcgill
3 years ago

Replaces bad diff in 31352.3.diff

#15 @joemcgill
3 years ago

I found a view that I missed when updating these media icons. 31352.4.diff adds support in the media list view (ignore 31352.3.diff). Just a reminder, you will still need to manually add the images from media-files-40px-optimized.zip.

This still isn't a complete patch, because we need to account for times where the icon directory has been filtered and the 2x versions do not exist. Not sure the best way to check if the there is a way to see if a filter has been set on the icon directory within the media/views.js, so it may be better to extend wp_get_attachment_image_src() to return a valid src and src-2x. On the other hand, if we were to just use SVGs instead of PNGs here, we could avoid adding a srcset at all and simplify the whole process.

Would be interested in feedback on the preferred way forward.

This ticket was mentioned in Slack in #core by drew. View the logs.


3 years ago

#17 @helen
3 years ago

  • Milestone changed from 4.2 to Future Release

Not yet on final approach here, too late for 4.2.

#18 @joemcgill
2 years ago

  • Milestone changed from Future Release to 4.5
  • Owner set to joemcgill
  • Status changed from new to accepted

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


2 years ago

#20 @chriscct7
2 years ago

  • Milestone changed from 4.5 to Future Release

Punting per discussion in Slack.

#21 @melchoyce
20 months ago

Would really love to see this get revisited soon.

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


12 months ago

#23 @melchoyce
12 months ago

@liljimmi, @empireoflight — This would be a good candidate for using SVGs.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


11 months ago

#25 @boemedia
4 weeks ago

  • Keywords has-ui-feedback needs-design added; ui-feedback removed

Totally agree this would be better in svg. Maybe @cathibosco or someone else from the dashicons team can take a peak? I'll ping them on Slack.

This ticket was mentioned in Slack in #design-dashicons by boemedia. View the logs.


4 weeks ago

#27 @boemedia
4 weeks ago

I mentioned this in the #design-dashicons channel on Slack yesterday, and according to @melchoyce , this now needs a developer to make a patch using the Dashicons svgs.

Note: See TracTickets for help on using tickets.