Custom Block Icons #8916

mtias · 2018-08-13T11:56:45Z

In Progress

This update seeks to add custom icons to the block library in order to reduce the expectation of dashicon usage on blocks, which has resulted in a loss of clarity as soon as multiple blocks share the same icon (quote and pullquote, for example). Dashicons were also designed for UI affordance while blocks have distinct qualities that we should reflect.

It also hopes to encourage people to supply a custom SVG for their own blocks.

Closes #8719.

The new icons have a 24px base grid (pulling from Material design). All core blocks have been replaced except for a few where we had custom icons already which need updating to the new sizing (shortcode and custom HTML). This also reworks all the icons used for embeds and favors service specific icons when available for clarity.

Missing Icons

Remaining Tasks

Address slash inserter.
Implement stacked icon deck.
Work on hover styles.
Define spread of background color.

chrisvanpatten · 2018-08-13T12:05:50Z

I love that this uses raw SVGs instead of some kind of special icon name thing. Encouraging raw SVGs is great.

Hopefully this also opens the door to improving those icons without worrying about matching an icon suite necessarily — that quote icon, for instance, feels like it could use a little love. Because it's raw SVG I imagine it'll be easier to make small case-by-case tweaks rather than trying to get Google to approve a design change upstream.

Super cool!

youknowriad · 2018-08-13T12:05:59Z

I think it's not easy to distinguish whether the title applies to the icon above it or under it. I think we can fix this by reducing the padding of the buttons and increasing the margin bottom of the items. (Something like the screenshot above)

Also, I don't like the fact that the scrollbars show up surrounded by a padding. I think it's better to avoid the global padding and move it to each child div instead, that way the scrollbars show up at the edge.

paulwilde · 2018-08-13T13:27:05Z

packages/block-library/src/image/index.js

+ </g>
+ </svg>
+);
+


This SVG seems to have a bunch of unnecessary IDs and tags.

karmatosed · 2018-08-13T17:23:21Z

@youknowriad regarding spacing, the issue and where tightness visually happens is adding background to icons. Here are some explorations:

ccprog · 2018-08-13T19:03:54Z

I just came across this PR and noted that some of the SVGs use the following pattern:

<svg width="24" height="24"><path fill="none" d="M0 0h24v24H0V0z"/>...

Note that there is no viewBox attribute, but instead an empty "backround" path element covering the whole viewport. If an icon defined like this is embeded as a <img>, its size can be styled. But if it is used inlined in HTML, changing the size with CSS does not work. You would be well-advised to use

<svg viewBox="0 0 24 24">...

for all icons to take advantage of SVGs intrinsic responsiveness, both to enable theming and for the sake of maintainablility. (All you'll ever have to do is adjust sizes in CSS.)

As an aside none of the other attributes sprinkled through there are really needed:

width and height should be and are provided by CSS,
version is never evaluated by browsers,
enable-background is deprecated,
xmlSpace is a valid attribute - in JSX -, but is superfluous for these icons, as they have no text content
xmlns="https://www.w3.org/2000/svg" is a matter of convention. It is not strictly needed for <svg> elements inlined in HTML, but if you ever copy-and-paste them to a file that is evaluated as XML (like a standalone SVG file), it is compulsory.

chrisvanpatten · 2018-08-13T19:17:50Z

It might be helpful to run the icons through svgo, which can help normalise some of the issues that @ccprog mentioned. I usually use this nice GUI option: https://jakearchibald.github.io/svgomg/

mtias · 2018-08-13T19:23:54Z

Thanks @ccprog for the tips! This is just a rough prototype at the moment. The default height / width was needed for the branch because in some cases there wasn't one provided by the CSS and it needed a fallback. They are to be removed before merging.

@chrisvanpatten I ran them through that tool at some point but it destroyed the shapes. Probably some wrong configuration, but we can probably just do the cleanup manually once we are ready.

ccprog · 2018-08-13T19:57:26Z

@chrisvanpatten SVGO is a heavily opinonated tool. In this case, it would remove the viewBox as a default. Despite what the GUI seems to claim, it cannot add a missing viewBox. Option "Prefer viewBox to width/height" only means it will dellete width/height if a viewBox is present. If it is not, a conscious designer decision is needed.

jasmussen · 2018-08-14T08:16:06Z

Pushed a little polish to the block chip, and the inline image icon:

youknowriad · 2018-08-14T08:19:04Z

What impact this PR should have on the block switcher's icon and the block style variations panel? (which mimic the inserter design)

mtias · 2018-08-14T10:21:10Z

@youknowriad it updates those.

jasmussen · 2018-08-14T13:01:12Z

Pushed a few fixes:

jasmussen · 2018-08-14T13:08:20Z

Normalized focus style:

jasmussen · 2018-08-15T13:37:17Z

In 5874f5b I pushed a little polish to focus and hover styles of blocks:

mtias · 2018-08-15T18:36:26Z

Oh, better structure for the scrollbar would also be needed.

jasmussen · 2018-08-15T18:48:38Z

I can fix it tomorrow so the scrollbar is right on the edge of the window as opposed to floating in the middle, if that's what you mean? I'll be sure to fix the padding also.

jasmussen · 2018-08-16T06:35:34Z

I pushed a change to the scrollable area:

Note I also changed the spacing from using 13px which didn't really fit with anything, to using 16px. I stored that as a new grid variable ($grid-size-large, with $grid-size being 8px), which we can use elsewhere to gain consistency.

jasmussen · 2018-08-16T09:14:43Z

Okay, I think I actually fixed the scrollbar.

jasmussen · 2018-08-16T09:36:59Z

Fixed the switcher as well:

jasmussen · 2018-08-16T09:50:37Z

Fixed style variations and focus styles.

paulwilde · 2018-08-16T11:53:43Z

Fixed the switcher as well:

The label text (Quote) looks too far to the bottom of the hover background in this image.

jasmussen · 2018-08-16T12:01:55Z

The label text (Quote) looks too far to the bottom of the hover background in this image.

This is sort of a side-effect of us removing the gray background around each icon. Look at the Columns block to see an outline of where they used to be.

One of the thigns to keep in mind is that blocks can color icons, and icon backgrounds, and this is later used in the block chip seen in the switcher and in the inspector. This is also used in the library, but no longer the gray background for icons that have no defined background. This is something I hope to revisit later on, to see if we can balance better so it isn't so overwhelming, but I feel like it might be good to get this out first as an iteration and then revisit.

chrisvanpatten · 2018-08-16T12:41:30Z

I definitely like the direction this is going but all that padding on the outside edge just feels weird vs having it extend full-width. It creates a weirdly raggedy edge…

vs

(EDIT: Just looking again at these screenshots side-by-side, it ironically feels like the version without the padding on the .editor-inserter__results panel actually feels more open and less crowded, because the icons and labels are a little further spaced. One of the original ideas behind this revision in #8719 was to add more space, but the padding kind of makes it feel more crowded to me.)

But I do like the new icons (feels super weird for WordPress to use Material Icons instead of a WordPress-specific icon set, but they still work) and the lack of a background on the default icons isn't as bothersome in practice as I thought it might be. Overall looks good!

youknowriad · 2018-08-16T13:12:12Z

I pushed a refactoring to BlockIcons rendering which hopefully 🤷‍♂️ does:

Fix adding embed blocks (always use BlockIcon to render icons)
Centralize the CSS to apply colors in BlockIcon using the showColors prop
Consistent rendering for BlockIcon with or without colors (always have a .editor-block-icon wrapper)
Remove contrast checking for block icons (fix e2e tests)

Uses custom svgs at a 24px base, 36px upscale for the inserter. WordPress should signal that block icons ought to be custom for the sake of clarity for users.

youknowriad · 2018-08-16T14:43:52Z

So tests pass, I think we're ready to ship. Could anyone test everything for last minute breakage?

@chrisvanpatten I'm not designer but I had the same thoughts, I feel without the padding, it's better but that's shouldn't block us here. It's still a huge improvement over what we have, we can merge and iterate.

youknowriad

LGTM 👍

chrisvanpatten · 2018-08-16T15:30:53Z

Only remaining thing I noticed, but shouldn't be a blocker, is that dashicons now render blurry in non-retina displays (they may also be blurry in Retina, but I expect not because pixels!) because they're upscaled. It'd be nice if there was a fallback that just padded the dashicons instead of upscaling. But again, that shouldn't be a blocker.

jorgefilipecosta · 2018-08-16T15:46:17Z

test/e2e/specs/block-icons.test.js

@@ -130,7 +130,7 @@ describe( 'Correctly Renders Block Icons on Inserter and Inspector', () => {
 it( 'Renders the icon in the inserter with the correct background color and an automatically compute readable foreground color', async () => {
 await searchForBlock( blockTitle );
 validateSvgIcon( await getFirstInserterIcon() );
- expect( await getBackgroundColor( INSERTER_ICON_SELECTOR ) ).toEqual( 'rgb(1, 0, 0)' );
+ expect( await getBackgroundColor( INSERTER_ICON_SELECTOR ) ).toEqual( 'rgba(0, 0, 0, 0)' );


The background color is in fact rgb(1, 0, 0) and with this change we don't verify if the background was in fact applied. I think we may need to update the selector const INSERTER_ICON_SELECTOR = ${ INSERTER_BUTTON_SELECTOR } .editor-block-icon; to be ${ INSERTER_BUTTON_SELECTOR } .editor-block-types-list__item-icon.

Oh gotcha I'll take a look

.editor-block-icon is a child of .editor-block-types-list__item-icon and it seems colors are applied in .editor-block-types-list__item-icon.

It should be fixed.

karmatosed

I think we should ship what we have right now. The indent visually does help for padding on scrolling. Let's get in as we have now and we can always iterate.

youknowriad · 2018-08-16T16:48:44Z

Awesome work everyone, I love those PRs where so many people push code and review :P

aduth

Curious the impact this might have on bundle size. Probably not critical impact, but I'm sure it's a bump.

aduth · 2018-08-16T16:49:30Z

packages/editor/src/components/block-icon/index.js

- { renderedIcon }
- </div>
- );
+ const style = showColors ? {


Wasted effort to assign variable when following condition shortcuts early return without using (should be assigned after condition).

As per the update in #8916, our core blocks now use Material Design icons. This PR updates our design doc about block design to reflect that.

afercia · 2018-08-23T10:51:17Z

With this change, all the new hardcoded SVG icons miss the attributes required for accessibility:
role="img" aria-hidden="true" focusable="false" 😞Will open a new issue.

jasmussen · 2018-08-23T10:54:03Z

You can assign that issue to me.

As per the update in #8916, our core blocks now use Material Design icons. This PR updates our design doc about block design to reflect that.

mtias added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Status] In Progress Tracking issues with work in progress labels Aug 13, 2018

mtias added this to the 3.6 milestone Aug 13, 2018

paulwilde reviewed Aug 13, 2018

View reviewed changes

packages/block-library/src/image/index.js Outdated

</g>

</svg>

);

Copy link

Contributor

paulwilde Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SVG seems to have a bunch of unnecessary IDs and tags.

mtias added 3 commits August 16, 2018 14:42

Custom icons for Blocks

9c3066b

Uses custom svgs at a 24px base, 36px upscale for the inserter. WordPress should signal that block icons ought to be custom for the sake of clarity for users.

Update some more icons.

59c6e00

Add icons for several core blocks.

b446507

Joen Asmussen and others added 6 commits August 16, 2018 14:43

Fix switcher

440fed5

Fix style variations.

3f0858b

Fix unit tests

0d1d7e7

Refactoring block icons rendering

a448618

Fix style lint

638213b

Remove debug code :) Sorry

f5ee80c

youknowriad force-pushed the add/custom-block-icons branch from cb665f2 to f5ee80c Compare August 16, 2018 13:43

youknowriad added 2 commits August 16, 2018 14:45

Fix icon styles

55c5294

Fix block icons e2e test

6c580c3

youknowriad approved these changes Aug 16, 2018

View reviewed changes

youknowriad removed the [Status] In Progress Tracking issues with work in progress label Aug 16, 2018

jorgefilipecosta reviewed Aug 16, 2018

View reviewed changes

Make sure the e2e icons test validates the colors properly

32a5a55

karmatosed self-requested a review August 16, 2018 16:42

karmatosed approved these changes Aug 16, 2018

View reviewed changes

youknowriad merged commit f9e82d0 into master Aug 16, 2018

youknowriad deleted the add/custom-block-icons branch August 16, 2018 16:48

aduth reviewed Aug 16, 2018

View reviewed changes

ghost mentioned this pull request Aug 19, 2018

Material icons (suggestion) elegantthemes/create-divi-extension#158

Closed

kjellr mentioned this pull request Aug 20, 2018

Update Facebook icon SVG #9178

Merged

kjellr added a commit that referenced this pull request Aug 22, 2018

Add mention of Material Design icons to the design docs.

ef48dd0

As per the update in #8916, our core blocks now use Material Design icons. This PR updates our design doc about block design to reflect that.

kjellr mentioned this pull request Aug 22, 2018

Add mention of Material Design icons to the design docs. #9240

Merged

afercia mentioned this pull request Aug 23, 2018

SVG icons: restore attributes required for accessibility #9269

Closed

karmatosed pushed a commit that referenced this pull request Aug 23, 2018

Add mention of Material Design icons to the design docs. (#9240)

0cae578

As per the update in #8916, our core blocks now use Material Design icons. This PR updates our design doc about block design to reflect that.

chrisvanpatten mentioned this pull request Sep 6, 2018

Created a new package containing all icons #9648

Closed

4 tasks

Custom Block Icons #8916

Custom Block Icons #8916

Conversation

mtias commented Aug 13, 2018 • edited by jasmussen Loading

Missing Icons

Remaining Tasks

chrisvanpatten commented Aug 13, 2018

youknowriad commented Aug 13, 2018

paulwilde Aug 13, 2018

Choose a reason for hiding this comment

karmatosed commented Aug 13, 2018

ccprog commented Aug 13, 2018

chrisvanpatten commented Aug 13, 2018

mtias commented Aug 13, 2018

ccprog commented Aug 13, 2018

jasmussen commented Aug 14, 2018

youknowriad commented Aug 14, 2018

mtias commented Aug 14, 2018

jasmussen commented Aug 14, 2018

jasmussen commented Aug 14, 2018

jasmussen commented Aug 15, 2018

mtias commented Aug 15, 2018

jasmussen commented Aug 15, 2018

jasmussen commented Aug 16, 2018

jasmussen commented Aug 16, 2018

jasmussen commented Aug 16, 2018

jasmussen commented Aug 16, 2018

paulwilde commented Aug 16, 2018

jasmussen commented Aug 16, 2018

chrisvanpatten commented Aug 16, 2018 • edited Loading

youknowriad commented Aug 16, 2018

youknowriad commented Aug 16, 2018

youknowriad left a comment

Choose a reason for hiding this comment

chrisvanpatten commented Aug 16, 2018

jorgefilipecosta Aug 16, 2018

Choose a reason for hiding this comment

youknowriad Aug 16, 2018

Choose a reason for hiding this comment

jorgefilipecosta Aug 16, 2018

Choose a reason for hiding this comment

youknowriad Aug 16, 2018

Choose a reason for hiding this comment

karmatosed left a comment

Choose a reason for hiding this comment

youknowriad commented Aug 16, 2018

aduth left a comment

Choose a reason for hiding this comment

aduth Aug 16, 2018

Choose a reason for hiding this comment

afercia commented Aug 23, 2018

jasmussen commented Aug 23, 2018

mtias commented Aug 13, 2018 •

edited by jasmussen

Loading

chrisvanpatten commented Aug 16, 2018 •

edited

Loading