Allow custom-size-icons

[jasmussen]

This fixes 9243.

This allows custom sized icons in the block library, by simply not overriding SVG provided dimensions. It works this way:

• If you provide a dashicon, its dimensions come from the SVG itself, because it has a dashicons class
• If you provide an icon without explicit dimensions, we count on it being a 24x24px icon
• If you provide a custom icon that does not have the dashicons class, and is not 24x24, you need to provide your own dimensions (width and height) in the SVG.

So as to keep some consistency, a max-width and max-height are applied, which kick in if a block icon larger than 24x24 is supplied.

☝️ in this screenshot, a 20x20px icon is provided for dev purposes to test this. This icon does not have a CSS class, but does have explicitly provided dimensions, so it's not scaled up.

[tofumatt]

Looks good/makes sense, but I have a question about the selector used to target SVGs we shouldn't adjust. It doesn't seem to match the comments.

[tofumatt]

Based on the comments, shouldn't this be:

svg:not(.dashicon),
svg:not([height]),
svg:not([width]) {

As it is an explicit height alone would still be overridden, and the docs imply that an explicit width OR height will prevent it–even if the dashicon class is there. That's how I read it.

[chrisvanpatten]

I think the 24px width/height needs to be applied to the container, otherwise the height of the whole item—when using a smaller icon—is wrong in the inserter:

[jasmussen]

As it is an explicit height alone would still be overridden, and the docs imply that an explicit width OR height will prevent it–even if the dashicon class is there. That's how I read it.

Your point that this comment needs clarification is solid. I got a headache just trying to walk through this now. I might need your help to clarify my code :D

So, this is what we want to accomplish:

• If you just dump in an SVG and don't really think about it, it's 24x24 as it should be
• If you dump in a dashicon, we know that's 20x20 so we don't enforce 24x24
• If you specify an explicit width (or height, do we have to check for that, seems like we could assume with a width?) — then sure, go ahead and have a custom sized icon so long as it's smalller than 24x24.

Hmm. Actually, your code should do the same shouldn't it?

How would you write this as a comment? Thanks so much for your help!

[tofumatt]

Same here re: selectors.

[chrisvanpatten]

I think the 24px width/height needs to be applied to the container, otherwise the height of the whole item—when using a smaller icon—is wrong in the inserter:

[chrisvanpatten]

So actually I just tried what I mentioned in my review comment, with the 24px width/height moved to .editor-block-icon. Weirdly, this caused the whole item (what are we calling these things? block chips? library items?) to shift below the baseline.

I'm reasonably sure this is related to display: inline-block but will need to dig in more…

[chrisvanpatten]

Okay, so here's what worked for me, but might have unintended consequences. Caveat emptor and all…

.editor-block-icon {
	display: block;
	border-radius: 4px;
	margin: 0 auto;
	height: 24px;
	width: 24px;

	&.has-colors {
		svg {
			fill: currentColor;
		}
	}

	// Don't allow icons to be bigger than the recommended standard.
	svg {
		max-width: 24px;
		max-height: 24px;
		vertical-align: middle;
	}
}

I tried using display: flex but that causes unintended consequences with SVGs without an explicit width/height set.

[jasmussen]

Thank you for the reviews!

When I try your code, Chris, it looks good:

However, when I try to drop in a 20x20 dashicon (with explicit widths and heights), as you say, the whole baseline is changed and the dashicon icon button is offset.

Whereas with the original code in my PR, all looks good to me:

The dashicon is 20, the other icons are 24, and all seems good.

I think the 24px width/height needs to be applied to the container, otherwise the height of the whole item—when using a smaller icon—is wrong in the inserter:

Help me understand this. I can't reproduce your issue with what's in this PR already.

[chrisvanpatten]

@jasmussen I’m not seeing any issues in your screenshot, but I’ll spin this back up and try again :)

EDIT: Oh, I see it now. Let me play around; I'm sure we can find a solution!

EDIT (26 August): Planning to pick this up tomorrow… a site launch had me side-tracked :)

[youknowriad]

I'm moving to 3.8 but feel free to merge if it makes the cut to 3.7

[chrisvanpatten]

I think I’ve got the problem pinned down and I think a solution involves another wrapper div. Would that be acceptable? I can keep iterating on some other ideas too but I think this will be easier.

[jasmussen]

I think I’ve got the problem pinned down and I think a solution involves another wrapper div. Would that be acceptable? I can keep iterating on some other ideas too but I think this will be easier.

I don't think that's a problem. And if you like, feel free to try and push your fixes to this branch.

[jasmussen]

@chrisvanpatten Do you think your solution would fix this small issue?

I.e. when the block is taller than stock, it messes with the floats.

[chrisvanpatten]

@jasmussen So I was actually able to solve both this issue (custom size icons) and the issue you pointed out. They are actually solved the same way, and I think it has to do with the fact that the inserter "blocks" are actually using display: inline.

If the whole thing is rebuilt using Flexbox, it all sort of magically works.

But, my fear is that this will inadvertently break something else.

At this point, is it worth possibly putting that in a separate PR to try out, since it's more than just allowing custom size icons?

[jasmussen]

Absolutely put that in a PR, it sounds better than my solution. You can assign me as reviewer and I'll take a look. We'll be sure to test it and help catch any side effects! Thank you.

[chrisvanpatten]

#9497 solves both issues (and hopefully doesn't create any new ones in the process!)

[tofumatt]

Please re-open this if I'm wrong, but you said #9497 closes this and it's merged… so closing this one for now 😄

[chrisvanpatten]

@tofumatt It originally closed it, but I undid those changes. Can you re-open? I've been meaning to get back to this, life just got in the way :)

EDIT: nvm, once I restored the branch I was able to reopen!

[chrisvanpatten]

Thanks for the reminder on this gents :)

@jasmussen and I had a conversation a little while back about this and a few points came up. I'm going to try to sum it all up as best I can. Note that this is largely philosophical. There are a number of ways we can solve this technically which I'll also explain, but before we fix this, we have to decide if we even want to.

Sorry for the following. It got long.

Should we "support" non-material icons?

The first question is whether or not we even want to encourage developers to use non-Material Design Icons for their blocks?

No, discourage non-material icons

There is certainly an argument for consistency in saying "no, block developers should always draw from the Material icons pool". You get a consistent visual style, and a sort of guarantee that your icon will continue to be correctly sized and formatted.

(Block devs using logos or other custom brand assets for their icon will still need to do some kind of custom prep, but they probably will need to do that regardless of the outcome here.)

Yes, allow non-material icons

However, I'd argue that as long as the primary way you add a block icons is by pasting in a raw SVG (as opposed to dashicons where you provide an icon name), block developers will always have a natural temptation to pull from other icon sets.

And in fact, I'd go as far as to encourage this. Allowing and encouraging devs to choose non-material icons helps reinforce a distinction between "core" blocks and 3rd party blocks.

It also helps in cases where you might create a block that replicates some core functionality. For example we recently created a "section heading" block. It's basically the core heading block with slightly tweaked markup and another option.

It would be easy to just use the same icon the core heading block uses, but it's a better user experience to use something visually distinct. However MDI only has so many icons that can represent "heading". So it's much easier for me to just grab something from another icon set.

If no…

If we decide to just commit to "yes, block developers should always draw from the Material icons pool" (which there are very valid reasons for!) then we can pretty much end here.

However…

If yes: how?

However, if we want to allow people to draw from other icon sets, we have to overcome another problem.

See, material design icons are designed to render out to 24x24 dp (resolution-independent px): 20dp "safe" area and a 2dp "bleed" on each side. Icons are allowed to extend into this bleed area so it's not just something you can crop out… the bleed area is essentially required.

Most other icon sets don't work like this; a bleed area around each icon is pretty unique to MDI.

Most other icon sets set a size and fill to that size. Dashicons is a good example: we currently have custom sizing for dashicons because they're designed to fit 20x20dp with no bleed. FontAwesome is similar: it's designed for 16x16dp, with no bleed area.

Outside of material design, an icon not filling the full canvas is the exception, not the rule.

Because most other icon sets don't do this, non-"correctly" sized icons will need to be somehow accommodated. The question is, do we handle this for users, or do we ask users to handle it?

Users handle it

If we ask users to handle it, this basically means modifying their icons in advance to fit the material design standards: that 20x20dp area with 2dp bleed.

However, my concern is that this is more complex than you might expect, because the tools for editing SVGs aren't super great.

• Illustrator exports weird markup/settings (ever tried exporting a clean viewbox? good luck).
• Inkscape is pretty cludgy… on Mac, for instance, the 0.92 series doesn't have any built packages available, and also you need to install XQuartz which most users won't already have
• Sketch is the best I've used, but isn't really designed to be an SVG editor, and is only available on Mac
• Pixelmator Pro technically supports SVGs, but it is also Mac-only and half the time can't open the SVG files I give it anyway

As a result, prepping non-material icons can be an unexpectedly frustrating experience, as I found when I was working on #9201.

Gutenberg handles it

It should be no surprise at this point that I prefer Gutenberg handling it. That's where this PR comes in.

I think the best approach would be:

1. Encourage block developers to add width/height attributes to their SVGs in all cases
2. Add default width/height attributes in <BlockIcon /> if they don't provide them, similar to how we handle SVG accessibility parameters. (This also helps us handle core block icons which don't have width/height attributes set, so we don't have to do CSS attribute selectors/:not() tricks)
3. Use flexbox to centre the icon inside the <BlockIcon /> component

Even if we continue to encourage folks to try picking from the Material Icon pool first, this approach has the added benefit of simplifying the BlockIcon CSS, because we wouldn't need to have the :not(.dashicon) ruleset. Width/height would be explicitly applied, so it would work inside flexbox.

Phew…

Anyway at this point I think what we need to move forward is feedback on whether or not this is a case we want to support, and who should be responsible for supporting it.

In my mind, handling it in Gutenberg is an easy win that will help diversify the set of available icons for 3rd party blocks without adding extra hoops for plugin developers. It also helps us avoid needing to give Dashicons special treatment; with this approach, all icons are treated equally in CSS.

However, it's ultimately not my call and I'm very interested to hear what others think :)

[jasmussen]

Thanks again Chris for your time and your thoughtful feedback.

Step 1 was completed when we got your flexy inserter in, that was awesome, thanks so much.

Step 2 is having this discussion, which I feel is totally worth it. However I would suggest that it's slightly more complex than these two cases:

1. No, discourage non-material icons
2. Yes, allow non-material icons

I would suggest the removal of the term "Material" from this, and refer instead to 24x24px icons. We don't look specifically for the icons to be Material, they can be any icon drawn on a 24x24px canvas, the prime argument here being that this is a good size that scales across mobile and desktop, is more legible than smaller icons, and uses a multiple of the 8px base grid that Gutenberg is designed on. See also WordPress/dashicons#255 (comment) for more thoughts on this.

Also, simply for compatibility reasons we will be allowing dashicons as well, although not quite as ideal, we don't want those blurry.

If we ask users to handle it, this basically means modifying their icons in advance to fit the material design standards: that 20x20dp area with 2dp bleed.
However, my concern is that this is more complex than you might expect, because the tools for editing SVGs aren't super great.

You could make this argument for any icon set, and this is not unique to material.

The reason for using Material here is that the icon set is open source and vast, so it should be easier to find an icon that fits you.

There is no way we can make sure that plugin developers create nice icons — this much we already know from plugins adding icons to the WordPress sidebar. All we can do is make sure whatever standards we choose are scalable for the future, and in this case the combination of 24px and the material open source set (or Gridicons for that matter) allows for a good start.

In my mind, it is more important for consistency that icons are sized the same, than it is they are visually the same. This is evident from logos like those from the embed blocks, when seen side by side with normal blocks.

But I don't know that Gutenberg can handle this — you can't just change the viewbox of an SVG and call it a day, the icon has to be redesigned or it's not going to look good. So yes, that means a stop through Inkscape or Illustrator. So that gives us this matrix:

1. User uses a 24px icon, like Material or Gridicons or something. All good.
2. User draws a 24px icon. All good
3. User uses a 20px dashicon. It's unscaled, it's okay but it's definitely a legacy thing.
4. User uses a random non-24px icon. It's scaled and might be blurry, not ideal.

If we spent a lot of work to morph the SVGs, at best we'd end up with use case 4 — blurry icons, or 3, smaller icons that are optically inconsistent with the rest of the icons. It doesn't seem like a win to me.

[jasmussen]

Closing this in favor of #9828.