New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use flexbox for the inserter layout #9497

Merged
merged 4 commits into from Sep 3, 2018

Conversation

Projects
None yet
5 participants
@chrisvanpatten
Contributor

chrisvanpatten commented Aug 31, 2018

This PR changes the inserter icon grid from using display: inline on each <li> to using a flexbox-based approach.

This solves a few things:

  • staggered icons due to varying heights
  • block icons smaller than 24px (the display: inline was causing issues with smaller icons shifting the baseline of the icon)

Here's a screenshot, with red borders applied, to show how the buttons and icons are all correctly sized and positioned.

image

I haven't done thorough testing in IE just yet, but I'm happy with how it's looking in Safari, Firefox, and Chrome.

At this point I'm very interested in just getting feedback on whether this approach is viable, and — in particular — any regressions that might have arisen as a result of this change.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

A few more screenshots:

fullscreen_31_8_18__08_34

fullscreen_31_8_18__08_35

Note that I've deliberately resized some of the block icons with width/height attributes, so you can see how smaller icons are handled smoothly. Those changes were just for testing and are not committed inside the PR :)

Contributor

chrisvanpatten commented Aug 31, 2018

A few more screenshots:

fullscreen_31_8_18__08_34

fullscreen_31_8_18__08_35

Note that I've deliberately resized some of the block icons with width/height attributes, so you can see how smaller icons are handled smoothly. Those changes were just for testing and are not committed inside the PR :)

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

Looks good now in IE11. Also tested in Edge and there were no issues there.

Contributor

chrisvanpatten commented Aug 31, 2018

Looks good now in IE11. Also tested in Edge and there were no issues there.

@jasmussen

Very nice work. This seems solid, except for one thing, now the switcher icon is a little broken:

screen shot 2018-08-31 at 15 07 59

It appears that the removal of the explicit width and height (https://github.com/WordPress/gutenberg/pull/9497/files#diff-4ef4fb3b2b17659317ef03b9f33c6d07L12) causes that icon to collapse. In other words, adding those rules back appear to fix it.

After your initial assessment about icon sizes, I was in fierce agreement: icons must never be blurry! Blur must be destroyed!

However, we're also laying the groundwork for years and years of plugin development, and perhaps we have a responsibility ensure some consistency here.

In other words, I'm coming back to the idea that we should perhaps enforce the dimensions of block icons in the switcher, and make an exception only when that icon is a Dashicon.

In other words, perhaps we should restore this rule:

svg:not(.dashicon) {
	width: 24px;
	height: 24px;
}

I feel like it's a fair tradeoff to require you to include the dashicon class in your SVG if you need to use a dashicon for your block. If you like, I can submit a PR to the Dashicons repo that ensures those classes are not stripped from the minified SVGs that the repo now comes with. Would that help?

@jasmussen jasmussen self-requested a review Aug 31, 2018

@jasmussen

Oops, I accidentally selected "Approve" when I meant to request changes. Sorry, got trigger happy!

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

In other words, I'm coming back to the idea that we should perhaps enforce the dimensions of block icons in the switcher, and make an exception only when that icon is a Dashicon.

I'm strongly opposed to that, because of that 2px "padding" that has to be added to make icons compatible with the Material icon grid. That would basically prevent anyone from just dropping in icons from any other icon set without going through the extra work of adjusting the SVG, which is not always an easy experience. Illustrator sucks for generating SVGs, Sketch is nice but Mac only (and $99), and Inkscape is slow and cumbersome.

I think the better solution is to set a specific width and height attribute on all the material SVG icons, and patch up the transform icon to accommodate this case in the meanwhile.

I'm on the case.

Contributor

chrisvanpatten commented Aug 31, 2018

In other words, I'm coming back to the idea that we should perhaps enforce the dimensions of block icons in the switcher, and make an exception only when that icon is a Dashicon.

I'm strongly opposed to that, because of that 2px "padding" that has to be added to make icons compatible with the Material icon grid. That would basically prevent anyone from just dropping in icons from any other icon set without going through the extra work of adjusting the SVG, which is not always an easy experience. Illustrator sucks for generating SVGs, Sketch is nice but Mac only (and $99), and Inkscape is slow and cumbersome.

I think the better solution is to set a specific width and height attribute on all the material SVG icons, and patch up the transform icon to accommodate this case in the meanwhile.

I'm on the case.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

To clarify, I'm not really concerned about dashicons in particular; I want it to be possible to use any icon set, ideally without needing anything more than an SVG and width/height attributes.

Honestly I think the "correct" solution here is to just add width/height attributes to all block icons in Gutenberg, because that allows us to use flexbox there as well. SVGs break in flexbox if there's no width/height assigned to the SVG… so rather than fight that, why not just add the attributes?

If the base icon set didn't have that padding (I know it's not really a padding, but it acts as one on most icons) it wouldn't be as big a concern because we would pad the icon in our CSS. But because the padding is built into the icon, adding an improperly prepared SVG and letting it scale up to 24x24 leads to a really ugly situation where you not only have an icon displayed at a strange size and looking blurry, but you also have no breathing room around the icon and it looks bigger than all other icons.

Contributor

chrisvanpatten commented Aug 31, 2018

To clarify, I'm not really concerned about dashicons in particular; I want it to be possible to use any icon set, ideally without needing anything more than an SVG and width/height attributes.

Honestly I think the "correct" solution here is to just add width/height attributes to all block icons in Gutenberg, because that allows us to use flexbox there as well. SVGs break in flexbox if there's no width/height assigned to the SVG… so rather than fight that, why not just add the attributes?

If the base icon set didn't have that padding (I know it's not really a padding, but it acts as one on most icons) it wouldn't be as big a concern because we would pad the icon in our CSS. But because the padding is built into the icon, adding an improperly prepared SVG and letting it scale up to 24x24 leads to a really ugly situation where you not only have an icon displayed at a strange size and looking blurry, but you also have no breathing room around the icon and it looks bigger than all other icons.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

I have to run out for the day soon, but one quick idea would be to enforce the width/height on the SVG, rather than in CSS. This would allow us to avoid CSS that targets specific icon sets and allow smaller icons to work seamlessly.

This would work by setting the "size" parameter on dashicons which will assign a width/height, and then updating the attributes in on the SVG component to set a width/height of 24 if the SVG doesn't already have them.

It feels "right" to me to just have the width/height on icon markup, rather than trying to futz around with attribute selectors and dashicon exceptions and all in CSS. Does that seem like a reasonable thing to try?

Contributor

chrisvanpatten commented Aug 31, 2018

I have to run out for the day soon, but one quick idea would be to enforce the width/height on the SVG, rather than in CSS. This would allow us to avoid CSS that targets specific icon sets and allow smaller icons to work seamlessly.

This would work by setting the "size" parameter on dashicons which will assign a width/height, and then updating the attributes in on the SVG component to set a width/height of 24 if the SVG doesn't already have them.

It feels "right" to me to just have the width/height on icon markup, rather than trying to futz around with attribute selectors and dashicon exceptions and all in CSS. Does that seem like a reasonable thing to try?

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

Okay in the interest of not derailing my own PR, I'm going to update it to not include icon changes, and only make the inserter flexboxy ;)

Contributor

chrisvanpatten commented Aug 31, 2018

Okay in the interest of not derailing my own PR, I'm going to update it to not include icon changes, and only make the inserter flexboxy ;)

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 31, 2018

Contributor

Thanks, I think your objections are totally fair, we can discuss them separately. Sorry for derailing that, and thanks again for the PR.

Contributor

jasmussen commented Aug 31, 2018

Thanks, I think your objections are totally fair, we can discuss them separately. Sorry for derailing that, and thanks again for the PR.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Aug 31, 2018

Contributor

Back in business without the icon changes. Lookin' goood… 👍

Contributor

chrisvanpatten commented Aug 31, 2018

Back in business without the icon changes. Lookin' goood… 👍

@jasmussen jasmussen self-requested a review Sep 3, 2018

@jasmussen

Given recent changes, I can't seem to break this. It seems solid. Very nice work.

@pento pento merged commit c9668e9 into WordPress:master Sep 3, 2018

2 checks passed

codecov/project 50.23% remains the same compared to 6406f62
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias added this to the 3.8 milestone Sep 3, 2018

@designsimply

This comment has been minimized.

Show comment
Hide comment
@designsimply

designsimply Sep 11, 2018

Contributor

Just dropping in a quick note to say I tested this in Firefox 62.0 on macOS 10.13.6 with text zoomed in a few times using the following steps.

  1. Go to View > Zoom > Zoom Text Only in Firefox.
  2. Press ⌘+ a few times to zoom the text.
  3. Click the block inserter to the left of an empty block and scroll through to see if you can find any spacing oddities.

Result: block library layout looks great!

screen shot 2018-09-10 at 7 15 11 pm
Seen at http://alittletestblog.com/wp-admin/post.php?post=14854&action=edit with text-nly zoomed 170% running WordPress 4.9.8 and Gutenberg 3.8.0-rc.1 using Firefox 62.0 on macOS 10.13.6.

Contributor

designsimply commented Sep 11, 2018

Just dropping in a quick note to say I tested this in Firefox 62.0 on macOS 10.13.6 with text zoomed in a few times using the following steps.

  1. Go to View > Zoom > Zoom Text Only in Firefox.
  2. Press ⌘+ a few times to zoom the text.
  3. Click the block inserter to the left of an empty block and scroll through to see if you can find any spacing oddities.

Result: block library layout looks great!

screen shot 2018-09-10 at 7 15 11 pm
Seen at http://alittletestblog.com/wp-admin/post.php?post=14854&action=edit with text-nly zoomed 170% running WordPress 4.9.8 and Gutenberg 3.8.0-rc.1 using Firefox 62.0 on macOS 10.13.6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment