Support sprites for admin menu icons in register_post_type and add_menu_page

[helen]

We should encourage developers to maintain a consistent and beautiful admin UI by making it easier to register sprites with black and white and color versions of icons for the admin menu, namely in register_post_type() and add_menu_page() (and its wrappers).

Related: We should also do an education push and create an external web tool to make it easy to generate a properly sized, colored, and positioned sprite.

Related tickets: #20036, #19886

[helenyhou]

Initial thought: have the menu_icon arg and $icon_url param take an array with a bool for whether or not it's a sprite, defaulting to false.

[PeteMall]

Also, screen icons and support for both admin themes.

[scribu]

+1 for the general idea.

We should make it easy to use a single sprite for multiple menu items. So, the API would look something like this:

register_post_type( 'foo', array(
  ...
  'sprite' => array( 'url' => 'http://.../admin-menu.png', 'column' => 1 )
) );

register_post_type( 'foo', array(
  ...
  'sprite' => array( 'url' => 'http://.../admin-menu.png', 'column' => 2 )
) );

Each column in the sprite would contain all the states necessary for a single menu item.

[azaozz]

Wouldn't it be a lot cleaner (and easier for plugin authors) if there is a param to pass some extra css that defines the icons/backgrounds instead of forcing them to create sprites with exact dimensions and icon positions? Also the css could include some other tweaks for the screen, menu, toolbar, etc.

[scribu]

Why exactly would you need a param inside register_post_type() to add CSS? Just hook into 'admin_enqueue_scripts'.

The problem is that the required CSS is horrid. Here's what you have to add for a single menu:

#toplevel_page_app-dashboard div.wp-menu-image img {
	display: none;
}

#toplevel_page_app-dashboard div.wp-menu-image {
	background-image: url('...images/admin-menu.png');
	background-position: -0px -33px !important;
}

#toplevel_page_app-dashboard:hover div.wp-menu-image,
#toplevel_page_app-dashboard.wp-has-current-submenu div.wp-menu-image {
	background-position: -0px -1px !important;
}

[ryanduff]

Agreed on the concept. But I think what's needed is a fundamental change in the way all the post type icons are displayed.

Passing an icon url in the post type registration and then having it fall back to the default if its not passed/available. Let core handle the messy css and just say that your icon size has to be x by x. Of course the default post type definitions in core should match and use this method instead of the current way.

KISS

[scribu]

Passing an icon url in the post type registration and then having it fall back to the default if its not passed/available.

That's already the case.

Let core handle the messy css and just say that your icon size has to be x by x.

I see no point in custom icon sizes. All icons have the same size. If you want it to be smaller, just leave some extra room in the sprite.

[azaozz]

Replying to scribu:

Why exactly would you need a param inside register_post_type() to add CSS? Just hook into 'admin_enqueue_scripts'.

That's how it works at the moment, right? And the css would need to be added one way or the other. Restricting it to couple of values passed from PHP and auto-generating the css afterwards seems quite limiting (and it could still be overloaded from 'admin_head', etc.).

A good enhancement would be to add an HTML ID derived from the CPT's name on the div that takes the icon as a background so it could be targeted easier with css.

[scribu]

Restricting it to couple of values passed from PHP and auto-generating the css afterwards seems quite limiting

We trade flexibility for convenience; that's what most APIs do. If someone doesn't want to use it, they're free to write the CSS by hand.

A good enhancement would be to add an HTML ID derived from the CPT's name on the div that takes the icon as a background so it could be targeted easier with css.

+1

[azaozz]

Replying to scribu:

We trade flexibility for convenience; that's what most APIs do...

Right. The problem in this case (as @helenyhou mentions in the ticket description) is that such sprite would have to match exactly the expected dimensions and the images (icons) inside it would have to match the expected positions.

That seems quite harder to do. Defining the background position(s) in css is easier. It also limits the sprite to having only these two icons. If a CPT includes a sprite with several images used in different places, another sprite only for the menu will have to be included making this inefficient.

[ryanduff]

Replying to scribu:

Passing an icon url in the post type registration and then having it fall back to the default if its not passed/available.

That's already the case.

Yup, forgot about that. Not ideal how its implemented but that's how it is now.

Next suggestion would be 2 separate params... one that handled the 32x32 screen icon and a second that handled a sprite for the two 16x16 icons for the menu. I think it'd be simpler/easier to leave these as a file per CPT instead of the intricacies of building a sprite sheet for a site. That way if a dev ads a new cpt there's no need to regenerate a full sprite sheet, instead generating one with just the 2 menu icons for that post type.

[helenyhou]

Replying to azaozz:

Replying to scribu:

We trade flexibility for convenience; that's what most APIs do...

Right. The problem in this case (as @helenyhou mentions in the ticket description) is that such sprite would have to match exactly the expected dimensions and the images (icons) inside it would have to match the expected positions.

Without the intention of slighting anyone, I would wager that quite a few developers have zero or nearly no concept of CSS sprites, much less creating and colorizing them. In parallel with a generation tool, I think this would actually make it more accessible to those who are not so well-versed in CSS and image manipulation but work with CPTs and other admin pages. This shouldn't be about forcing sprites on anybody, just making it easier to utilize them.

Supporting columns (multiple menu icon sprites in one sheet) similarly would not lock anybody into using a single sheet for all icons, just make it easier. Yes, to use the API rather than your own CSS, you'd have to stick to a specific size and structure, but I don't think that's such a bad thing, especially with the possibility of a generator and/or at least a Photoshop template.

I would think that at least for the menu icon (not really thinking about the screen icon at the moment - blame that on petemall) we could just use the same menu_icon and $icon_url arg/param, as I mentioned in my first comment. Something like array( string $url = null, bool $sprite = false, int $col = null ), with $col being used to calculate horizontal positioning if specified.

[empireoflight]

A sample psd file with basic effects used in WP dashboard icons-can be used to make icons that look like the existing ones

[empireoflight]

FWIW I posted a psd file that plugin developers can use to design 16x16 icons that are similar to the ones in the dashboard.

[helenyhou]

Related (API for admin menu): #12718

HiDPI support seems to make having core support be even more desirable, even though the same results can be accomplished now.

[bradyvercher]

I came up with a concept for attempting to solve this issue awhile back on a smaller scale, but thought it might be useful here. Basically, the current behavior would continue to work, but if a developer passed an array to menu_icon (or even screen_icon), then CSS would be generated for a sprite. The array would simply contain the url as it's first argument, then a set of coordinate adjustments as the second argument, and if desired, a third argument would be the coordinate adjustments for a hover state. It'd work with a single sprite or individual sprites for each image.

It's similar to some of the concepts mentioned, but might be a little more flexible than a rigid template. My code is ​in this plugin for anyone interested in taking a look and seeing if it'd be of any use.

[azaozz]

Replying to bradyvercher:

...if a developer passed an array to menu_icon (or even screen_icon), then CSS would be generated for a sprite.

This would be about the same as outputting the actual css in the head, but more limiting. Imho it's easier/cleaner to have:

function my_css() {
  ?>
  <style>
  #my-id {
    background: url("...") no-repeat -20px -40px;
  }
  @media only screen and (-webkit-min-device-pixel-ratio: 1.5) {
    #my-id {
      background-image: url("...-2x");
      background-size: 100px 50px;
    }
  }
  </style>
  <?php
}
add_action('admin_head', 'my_css');

Don't think any plugin dev would have problem learning how to write that bit of css. Perhaps more helpful will be to have a good codex page describing how to write it and make the sprite(s).

[bradyvercher]

Perhaps I'm complicating this (that tends to happen sometimes), but this appears to be the amount of CSS required to get menu icons and screen icons with a standard state, hover/active states, and retina support when you call register_post_type() and add_menu_page(), in essence, matching the current default styles and behavior:

<style type="text/css">
#icon-edit.icon32-posts-my_post_type { background: transparent url(../images/icons32.png) no-repeat -133px -14px;}

#adminmenu #menu-posts-my_post_type div.wp-menu-image { background: transparent url(images/icons16.png) 0 0 no-repeat;}
#adminmenu #menu-posts-my_post_type div.wp-menu-image { background-position: 0 -16px;}
#adminmenu #menu-posts-my_post_type:hover div.wp-menu-image,
#adminmenu #menu-posts-my_post_type.wp-has-current-submenu div.wp-menu-image { background-position: 0 -16px;}

@media only screen and (-webkit-min-device-pixel-ratio: 1.5) {
	#icon-edit.icon32-posts-my_post_type { background-image: url(images/icons64.png); background-size: 32px 32px;}
	#adminmenu #menu-posts-my_post_type div.wp-menu-image { background-image: url(images/icons32.png); background-size: 16px 16px;}
}


#icon-my-plugin-page { background: transparent url(images/icons32.png) no-repeat 0 0;}

#adminmenu #toplevel_page_my-plugin-page div.wp-menu-image { background: transparent url(images/icons16.png) 0 0 no-repeat;}
#adminmenu #toplevel_page_my-plugin-page:hover div.wp-menu-image,
#adminmenu #toplevel_page_my-plugin-page.wp-has-current-submenu div.wp-menu-image { background-position: 0 -16px;}
#adminmenu #toplevel_page_my-plugin-page div.wp-menu-image img { display: none;}

@media only screen and (-webkit-min-device-pixel-ratio: 1.5) {
	#icon-my-plugin-page { background-image: url(images/icons64.png); background-size: 32px 32px;}
	#adminmenu #menu-posts-my_post_type div.wp-menu-image { background-image: url(images/icons32.png); background-size: 16px 16px;}
}
</style>

Of course you can combine the media queries, but the specificity is required to override the default styles (not sure if an ID on the icon element would address this), the ID on the menu item isn't consistent due to the items being registered different ways, and the default 'generic.png' has to be hidden when using add_menu_page(). That's a whole lot of CSS that is easy to get wrong and can be fairly complicated to decipher, especially for only two menu icons. In my opinion, it's a prime candidate for generation.

This is how it'd work in the approach I'm advocating and I'm totally willing to admit it has room for improvement (named arguments?):

<?php
add_menu_page(
	$page_title,
	$menu_title,
	$capability,
	$menu_slug,
	$function,
	array(
		$image_url,
		'x,y', // standard coordinates
		'x,y', // hover/active coordinates
		'x,y', // retina coordinates
		'x,y' // retina hover/active coordinates
	)
);

register_post_type( 'my_post_type', array(
	'menu_icon' => array( $image_url, 'x,y', 'x,y', 'x,y', 'x,y' ), // same arguments as 'add_menu_page()'
	'screen_icon' => array( $image_url, 'x,y', 'x,y' ), // standard, retina
) );
?>

I'd argue that's much cleaner than the CSS approach and what exactly is limiting about it? For any scenario that it doesn't account for, admin_head will continue to provide the ability to overload the styles.

So I don't see any harm in creating a simple, consistent API that does most everything any developer is going to need and that's also backwards compatible--unless I'm missing something obvious (which might be likely).

Basically an approach similar to this would allow for devs to keep their code cleaner, provide a consistent API to implement new icons more easily and quickly, is backwards compatible, and it can be overridden in admin_head.

[scribu]

I agree that that blob of CSS should be generated and being in Core would ensure that it's kept up to date.

	array(
		$image_url,
		'x,y', // standard coordinates
		'x,y', // hover/active coordinates
		'x,y', // retina coordinates
		'x,y' // retina hover/active coordinates
	)

Named arguments pls!

	array(
		'image_url' => $image_url,
		'standard' => 'x,y',
		'standard_hover' => 'x,y',
		'hidpi' => 'x,y',
		'hidpi_hover' => 'x,y'
	)

[kovshenin]

They would still need to provide the exact sizes of their sprites for hidpi, and I also think it's better to use two different image files, so something this:

register_post_type( 'my_post_type', array(
    'menu_icon' => array(
        'image_url'                     => $image_url,
        'image_url_2x'                  => $image_url_2x,
        'background_position'           => '100, 200',
        'background_position_hover'     => '300, 400',
        'background_position_2x'        => '200, 400', // if not specified just multiply background_position by two
        'background_position_hover_2x ' => '600, 800', // ditto
    ),
) );

Back-compat is just a matter of checking whether menu_icon is_string and adding the opacity and background-size things. Any suggestions? I can make a patch for this.

[scribu]

Multiplying the background position isn't usually correct. You don't need to have 200px of padding between icons.

How about this syntax:

'menu_icon' => array(
	'regular' => array(
		'image_url'                 => $image_url,
		'background_position'       => '100, 200',
		'background_position_hover' => '300, 400',
	),
	'2x' => array(
		'image_url'                 => $image_url_2x,
		'background_position'       => '100, 300',
		'background_position_hover' => '300, 500',
	),
),

[kovshenin]

Replying to scribu:

Yeah maybe there's some other secret formula that would fit most cases, but we can talk about that later. I like your approach of having two arrays, but I also think that if 2x is not needed, maybe we can let developers not type "regular" and work as a single array too:

'menu_icon' => array(
	'image_url'                 => $image_url,
	'background_position'       => '100, 200',
	'background_position_hover' => '300, 400',
),

Also just thinking out loud here about how would most sprites be composed. Since the menu icon is 16x16 and the screen icon is 32x32, it makes sense to stick both sizes in one sprite, which will give menu + screen icon and hidpi menu icons assuming the screen icon is just a 2x copy of the menu icon and not a totally different grapihc. This means, that at the cost of another 64x64 pixels (+ the padding) in that very same sprite, they can serve both regular and 2x off of one file, as opposed to creating two different files for regular and 2x.

I'll make an initial patch so we can keep discussing this based on something.

Thanks for your input!

[kovshenin]

Replying to scribu:

Multiplying the background position isn't usually correct. You don't need to have 200px of padding between icons.

You're right! When the 2x image is exactly 2x larger than the original, the background positions will remain the same. It's only the background-size that's going to have to scale down, so we'll probably need that as the fourth argument to the 2x array.

[kovshenin]

Allows to use an array (or even two) for sprites in menu_icon for register_post_type

[kovshenin]

In 20052.diff​:

First pass at handling an array (or two) for custom post types menu_icon. Here's ​a gist with three post types and different menu_icon for each, will help test and debug. I don't own a Retina device, but here's a screenshot from my iOS emulator running Retina mode: ​http://cl.ly/image/0n1w362L3Z15

Feedback welcome :)

[bradyvercher]

The use of two arrays looks good to me, but I don't think it should be a requirement to use two separate images. The point of sprites is to reduce requests, so another image will add another request (multiplied by however many menu icons and potentially screen icons use this technique).

For 2x support, the only required argument in that array should be 'background_size'. The image and coordinates should both fall back to the default array if they don't exist, but the ability to overload them should be there, if needed, for flexibility.

I didn't examine the patch closely, but it looks like it breaks backwards compatibility if an image url is passed, and it doesn't appear to account for add_menu_page().

[kovshenin]

Replying to bradyvercher:

Thanks for your feedback. None of the arguments to 2x are required, every single one is optional including background_size. This allows us to use a single image and just have different background positions for regular and 2x, like I described in my comment earlier.

If you take a look at the ​gist I published you'll see the three post types with three different approaches to menu_icon, and the first one is an image URL passed as a string. The back-compatibility code is at around line 262. If it's not working for you for some reason, please let me know.

With regards to add_menu_page and screen_icon, I'll just reuse the same code, as soon it's working well for menu_icon in register_post_type :)

[bradyvercher]

I gotcha and see what you're doing now regarding backwards compatibility. The thing is that most of those changes to the existing code are unnecessary and the back-compat check in admin_head can be removed. All the patch really needs to be concerned with (in this instance, regarding post types) is whether or not 'menu_icon' is an array, and if so, generate the CSS to overload the current styles. Just my thoughts on the current approach.

A couple of other things I noticed:

• The 'background_size' argument shouldn't ever really be necessary for the regular icon.
• Selectors need to be added for the current/active state for both sizes (when the menu item is selected -- .wp-has-current-submenu).
• I think explode() would be preferred to preg_split().
• You can probably continue if the '2x' array isn't set.
• Maybe I'm being dense here, but I can't imagine a scenario when 'background_size' wouldn't be required for retina support, so maybe continue if that isn't set, too?
• The logical comparisons should be switched according to the coding standards: if ( 2 == count( $xy ) )

Aside from that, it looks like some of the selectors being output in the head could potentially have a bunch of whitespace if optional arguments aren't defined and some of them may not even have any rules.

I don't mean to be overly nitpicky, it looks like a good start, this is just something I'd definitely like to use in the future instead of the current method!

[azaozz]

Replying to bradyvercher:

... this appears to be the amount of CSS required to get menu icons and screen icons with a standard state, hover/active states, and retina support when you call register_post_type() and add_menu_page(), in essence, matching the current default styles and behavior...

This CSS block appears to be using different images and sprites and some of the styles are repeating. If this had to be set by an API, you wouldn't be able to do that and would need to make new sprite.

So I don't see any harm in creating a simple, consistent API that does most everything any developer is going to need...

There are 7-8 different ways this can be done in CSS, if that API would need to cover all it would be neither simple nor clear.

The biggest problem with APIs that automatically output code in another language (i.e. PHP API for CSS code) is that the plugin developers would have to learn how the CSS works (so they know what values to pass in), and then how the API works. So instead of making something simpler, it makes it more complicated and demands more from the developers. On top of that there are always limitations when any type of code is auto-generated.

[kovshenin]

Replying to bradyvercher:

All the patch really needs to be concerned with (in this instance, regarding post types) is whether or not 'menu_icon' is an array

That's exactly what it's doing, and if it's a string (back-compat) then it's converting it to an array. Try and apply the patch to the current codebase and check the gist I posted to see how it works.

The logical comparisons should be switched according to the coding standards

Maybe, though yoda conditions are more common when comparing a variable, the current code compares a literal to a function, so it doesn't really matter, but we can then iterate on making the code cleaner. The current patch is more of a proof-of-concept and not a commit candidate.

Replying to azaozz:

I think you're right, after writing this patch and after doing it manually in a plugin, there really isn't any difficulty of rolling your own admin_head, in fact the latter feel easier than having to learn new function arguments. However the misleading part remains, the menu_icon argument, which I'd really like to use but ended up not doing so, both for the admin menu and screen icons, because it generates images, not divs I'd want to use.

Maybe we should just write some clear instructions in the Codex about hovers and 2x. Thoughts?

[scribu]

The problem here is actually too much flexibility. We can make it less complex by mandating a precise sprite layout, i.e. the layout that is used in the Core sprite.

[helenyhou]

Replying to scribu:

The problem here is actually too much flexibility. We can make it less complex by mandating a precise sprite layout, i.e. the layout that is used in the Core sprite.

Yes, please. This was intended as a simplification that does not add much to how things are currently registered - follow a template to make your sprite, give the menu_icon arg a name (and maybe just a true/false flag for 2x and a pre-determined naming scheme), and be done. Complex implementations where pixel locations are still needed to be known might as well be done in CSS by the dev. Things like columns in a single sprite might also be nice-to-have, but I can't see that being needed by plugins/themes all that often.

[azaozz]

Replying to kovshenin:

... However the misleading part remains, the menu_icon argument, which I'd really like to use but ended up not doing so, both for the admin menu and screen icons, because it generates images, not divs I'd want to use.

Passing the string 'div' instead of an url as $icon_url in add_menu_page() would output the same padded div as used in the default top menu items where a background can be set with css.

If there must be an automated way of doing this, I agree with @scribu and @helenyhou: that should require a sprite with specific dimensions. We can have a "template image" for it in the codex, etc.

Then passing an array to 'menu_icon' in register_post_type() should leave out menu-icon-post class from the <li> and the icon-edit id from the screen icon. The array can support passing the actual css which will simplify all this or the URLs to the sprites (assuming there is 2x sprite). Passing an empty array would leave the menu icon and screen icon without backgrounds so it's easier to use custom css there.

[bradyvercher]

I was already too far into creating a concept to turn back, so I'll go ahead and attach it to gauge interest. Default coordinates for a template could easily be built in. Column support could easily be added as well since that would just require calculating coordinates to feed into the algorithm.

[kovshenin]

Replying to azaozz, helenyhou and scribu:

Sound fair. Would a standard template be 28x54 pixels image and twice as large for 2x? If that's the case we can also have a default for the background size argument, which would leave the regular url (and 2x url for 2x) as the only required fields. If we want columns we'll need an optional columns arg that should go together with a background size (or columns count?)

'menu_icon' => array(
	'image_url' => '.../sprite.png',
),

'menu_icon' => array(
	'image_url' => '.../sprite.png',
	'image_url_2x' => '.../sprite-2x.png',
),

'menu_icon' => array(
	'image_url' => '.../sprite.png',
	'image_url_2x' => '.../sprite-2x.png',
	'size' => '84, 56', // three columns,
	'column' => 2,
),

Or maybe drop the size and use columns instead:

'menu_icon' => array(
	'image_url' => '.../sprite.png',
	'image_url_2x' => '.../sprite-2x.png',
	'columns' => 3, // 84x56 image
	'column' => 2,
),

Would this be simple enough to understand and use?

[griffinjt]

Replying to kovshenin:

Would this be simple enough to understand and use?

I think the columns idea is a little bit confusing, especially for those who have never worked with sprites before. I personally wouldn't mind it, but I've worked with sprites a bunch so I'm not an unbiased opinion.

[helen]

Less of a thing now what with icon fonts and painting SVG, I guess. Closing unless somebody still sees a need for it.

[andddd]

Can you guys at least fix IMG width and height so I could pass large enough picture to cover target devices?