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

Enforce a default icon size to allow flex alignment + fix unaligned labels #9828

Merged
merged 8 commits into from Sep 20, 2018

Conversation

Projects
None yet
3 participants
@chrisvanpatten
Contributor

chrisvanpatten commented Sep 12, 2018

This is designed to fix a problem where blocks using dashicons have their labels misaligned, which you can see in these crude illustrations:

edit_post_ paleo_meal_plans _wordpress

edit_post_ mindful _wordpress

In order to fix this, editor-block-icon needs a consistent size set, and the <svg> inside needs to be centred within it.

The right way to do this would be .editor-block-icon { width: 24px; height: 24px; display: flex; align-items: center; }.

The problem, however, is that SVGs that do not have width/height set inside a flex container disappear because their width/height is implicitly calculated by the browser as 0px.

I believe the best way to solve that problem, without CSS hacks, is to simply enforce width/height attributes inside the <BlockIcon /> component:

const appliedProps = {
	...icon.props,
	width: icon.props.width || 24,
	height: icon.props.height || 24,
};

return <AccessibleSVG { ...appliedProps } />;

For dashicons, I just used the Dashicon component's "size" attribute. This is set to 20 by default already, but I think the bit of explicitness in our code is a good thing.

Enforcing a width/height on the SVG allows us to use flexbox to position the SVG inside .editor-block-icon, which is awesome. It means editor-block-icon can be resized in any context, with the icons retaining their size.

This required tweaks in the various places we present block icons: inserters/autocompleters, block inspector, transform toolbar button, etc. But in many of these cases, I was actually able to simplify or unify the CSS. The transform icon in particular has a lot of red in the diff; it's much easier to position the block icon and dropdown icon with flexbox than with absolute positioning.

On icon sizing

Okay, so I'll admit it: this also fixes #9243 and supplants #9263.

BUT… that wasn't originally the point of this, for real! It was genuinely designed to fix the label alignment, but I realised along the way that the only sane way to do that is with width/height attributes on the SVG. Positioning the icons with flexbox has so many benefits but it requires having explicit width/height attributes on the SVGs.

It's still totally fair to encourage block developers to prepare icons "correctly", and I think that it's worth creating some documentation around the icon expectations (20dp + 2dp "bleed").

I just don't want this to be necessarily perceived as being a PR about block icon sizing because that was genuinely a side effect, not the intention :)

Looking forward to getting eyes on this!

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 12, 2018

Contributor

Here are a few "after" screenshots, although ideally the "after" is nearly identical to the "before" :)

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

Contributor

chrisvanpatten commented Sep 12, 2018

Here are a few "after" screenshots, although ideally the "after" is nearly identical to the "before" :)

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

edit_post_ paleo_meal_plans _wordpress

@chrisvanpatten chrisvanpatten changed the title from Enforce a default icon size to allow flex alignment + fix unaligned text to Enforce a default icon size to allow flex alignment + fix unaligned labels Sep 12, 2018

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 12, 2018

Contributor

I also want to add, in the interest of full transparency, that we could technically make this work without allowing devs to add SVGs smaller than 24px. It would just mean modifying <BlockIcon /> from this:

const appliedProps = {
	...icon.props,
	width: icon.props.width || 24,
	height: icon.props.height || 24,
};

return <AccessibleSVG { ...appliedProps } />;

to this:

const appliedProps = {
	...icon.props,
	width: 24,
	height: 24,
};

return <AccessibleSVG { ...appliedProps } />;

That's genuinely the only difference: allowing a block to provide their own width/height, vs overriding it. Otherwise, there's nothing in the CSS that is there to explicitly support custom sized icons.

So it is technically possible to accomplish the label alignment fix without fixing #9243. But the fix itself is so small, and doesn't impact CSS at all, and I couldn't resist adding it and testing the waters 😁

Contributor

chrisvanpatten commented Sep 12, 2018

I also want to add, in the interest of full transparency, that we could technically make this work without allowing devs to add SVGs smaller than 24px. It would just mean modifying <BlockIcon /> from this:

const appliedProps = {
	...icon.props,
	width: icon.props.width || 24,
	height: icon.props.height || 24,
};

return <AccessibleSVG { ...appliedProps } />;

to this:

const appliedProps = {
	...icon.props,
	width: 24,
	height: 24,
};

return <AccessibleSVG { ...appliedProps } />;

That's genuinely the only difference: allowing a block to provide their own width/height, vs overriding it. Otherwise, there's nothing in the CSS that is there to explicitly support custom sized icons.

So it is technically possible to accomplish the label alignment fix without fixing #9243. But the fix itself is so small, and doesn't impact CSS at all, and I couldn't resist adding it and testing the waters 😁

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

You present a solid, solid argument.

PR is working amazingly well, btw:

screen shot 2018-09-13 at 10 27 48

I absolutely think this PR should be merged. Zero objections. The code seems solid to me (though might want to expand the review range to gutenberg/core for an extra sanity check).

But yes, there is the aspect of icon sizing too ;) and I hope you'll think it's fair that I boil it down to this: should we be agnostic about what icons developers can use for their blocks? Or should we enforce icon sizes to encourage consistency?

Let me ask a few quick questions:

  • What would happen if a block developer add a 50x50px icon?
  • What would happen if a block developer add a 12x12px icon?
  • Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

You can see where I'm going with this. I'm not suggesting every icon should be a material icon, or even a gridicon. I'm not suggesting we should discourage 3rd party icon designs. I'm suggesting that we have a responsibility for the next ten years of WordPress to ensure some good ground rules that can encourage great block icons, and discourage abuse.

What do you think?

Contributor

jasmussen commented Sep 13, 2018

You present a solid, solid argument.

PR is working amazingly well, btw:

screen shot 2018-09-13 at 10 27 48

I absolutely think this PR should be merged. Zero objections. The code seems solid to me (though might want to expand the review range to gutenberg/core for an extra sanity check).

But yes, there is the aspect of icon sizing too ;) and I hope you'll think it's fair that I boil it down to this: should we be agnostic about what icons developers can use for their blocks? Or should we enforce icon sizes to encourage consistency?

Let me ask a few quick questions:

  • What would happen if a block developer add a 50x50px icon?
  • What would happen if a block developer add a 12x12px icon?
  • Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

You can see where I'm going with this. I'm not suggesting every icon should be a material icon, or even a gridicon. I'm not suggesting we should discourage 3rd party icon designs. I'm suggesting that we have a responsibility for the next ten years of WordPress to ensure some good ground rules that can encourage great block icons, and discourage abuse.

What do you think?

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 13, 2018

Contributor

What would happen if a block developer add a 50x50px icon?

It will be downscaled to 24x24, but the same will actually happen in master even without this PR.

What would happen if a block developer add a 12x12px icon?
Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

I'm totally okay with this, and doing it would mean the 12x12 icon would be scaled up to 20x20!

Contributor

chrisvanpatten commented Sep 13, 2018

What would happen if a block developer add a 50x50px icon?

It will be downscaled to 24x24, but the same will actually happen in master even without this PR.

What would happen if a block developer add a 12x12px icon?
Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

I'm totally okay with this, and doing it would mean the 12x12 icon would be scaled up to 20x20!

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

What would happen if a block developer add a 12x12px icon?
Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

I'm totally okay with this, and doing it would mean the 12x12 icon would be scaled up to 20x20!

That works for me. Flagging for some additional design thoughts and then we have a path forward! Thanks for your patience and resilience.

Contributor

jasmussen commented Sep 13, 2018

What would happen if a block developer add a 12x12px icon?
Would you be okay with a compromise where we added a min-width: 20px; and max-width: 24px; rule? :D

I'm totally okay with this, and doing it would mean the 12x12 icon would be scaled up to 20x20!

That works for me. Flagging for some additional design thoughts and then we have a path forward! Thanks for your patience and resilience.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 13, 2018

Contributor

Thanks @jasmussen! I've updated the PR and also added a spot of inline documentation.

(Also, where does one get the Avocado block? Sounds delicious… 🥑)

One final thought is that with the SVGs it is possible that someone could add a custom class to their SVG and mess around with it via CSS.

I don't think it's necessarily right for this PR to address that, but I think it would be theoretically possible to do what I did with width/height defaults for the className, except that we just enforce a particular class and ignore user-provided values.

Contributor

chrisvanpatten commented Sep 13, 2018

Thanks @jasmussen! I've updated the PR and also added a spot of inline documentation.

(Also, where does one get the Avocado block? Sounds delicious… 🥑)

One final thought is that with the SVGs it is possible that someone could add a custom class to their SVG and mess around with it via CSS.

I don't think it's necessarily right for this PR to address that, but I think it would be theoretically possible to do what I did with width/height defaults for the className, except that we just enforce a particular class and ignore user-provided values.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 13, 2018

Contributor

(Also, where does one get the Avocado block? Sounds delicious… 🥑)

Well, avocado is the key ingredient in guacamole. And I've recently been testing a PR by Riad that is so cool, I would call it holy guacamole. :D

Contributor

jasmussen commented Sep 13, 2018

(Also, where does one get the Avocado block? Sounds delicious… 🥑)

Well, avocado is the key ingredient in guacamole. And I've recently been testing a PR by Riad that is so cool, I would call it holy guacamole. :D

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 14, 2018

Contributor

I've just updated this to accommodate the changes from #9687, and updated the new dropdown-arrow mixin to position via Flexbox instead of absolute positioning in the switcher and other dropdowns!

Contributor

chrisvanpatten commented Sep 14, 2018

I've just updated this to accommodate the changes from #9687, and updated the new dropdown-arrow mixin to position via Flexbox instead of absolute positioning in the switcher and other dropdowns!

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 17, 2018

@jasmussen jasmussen added this to the 4.0 milestone Sep 17, 2018

@jasmussen jasmussen referenced this pull request Sep 17, 2018

Closed

Allow custom-size-icons #9263

@tofumatt

This all looks good, but why bother with the tiny leeway (20px-24px) in this PR? Why not just make 24px the size and be done with it. The code complexity to accommodate a minor visual inconsistency when we could just say "icons are 24px" and be done with it seems too much.

I say we go for the "size everything down/up to 24px" approach. If a developer ships an icon smaller than 24px we'll upsize it and it might look blurry but at least it'll be consistent.

Otherwise the approach seems good though, so once we just go with 24px I'm cool to ship this.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt because you can't size icons at 24px, that's the whole point. You need a 2px consistent padding/spacing around the icon, which material icons do by default, but most other icon systems don't.

Contributor

chrisvanpatten commented Sep 19, 2018

@tofumatt because you can't size icons at 24px, that's the whole point. You need a 2px consistent padding/spacing around the icon, which material icons do by default, but most other icon systems don't.

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

This is from the material design docs:

system_icons_-_material_design

This is designed to make it easier to use an icon that was prepared without that spacing, which I think is onerous for all the reasons listed here.

For further clarification, upsizing to 24px just isn't great because it removes that trim area that we expect to be there. So the icon just looks like it's taking up way too much space, and it looks wonky. This ensures that icons that are not properly prepared are only upsized to fill the live area, without also expanding into the trim area.

Contributor

chrisvanpatten commented Sep 19, 2018

This is from the material design docs:

system_icons_-_material_design

This is designed to make it easier to use an icon that was prepared without that spacing, which I think is onerous for all the reasons listed here.

For further clarification, upsizing to 24px just isn't great because it removes that trim area that we expect to be there. So the icon just looks like it's taking up way too much space, and it looks wonky. This ensures that icons that are not properly prepared are only upsized to fill the live area, without also expanding into the trim area.

right: 6px;
margin-left: $grid-size-small;
// This gives the icon space on the right side consistent with the material

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt are there any standards documented around ideal line length for comments like this?

@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt are there any standards documented around ideal line length for comments like this?

This comment has been minimized.

@tofumatt

tofumatt Sep 19, 2018

Member

No, sorry! Basically the rule is most of us set our visible tab length to two/four-width so I just made it work according to that size. I dunno, it probably was me being overly picky; I mostly just wanted to add the full-stop 😅

@tofumatt

tofumatt Sep 19, 2018

Member

No, sorry! Basically the rule is most of us set our visible tab length to two/four-width so I just made it work according to that size. I dunno, it probably was me being overly picky; I mostly just wanted to add the full-stop 😅

This comment has been minimized.

@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt No no it's totally fine! Truthfully I'm never sure and honestly go back and forth on this myself. I think it's worth trying to keep it standardised where possible. I work in a 120 col terminal though so this line length is totally fine.

@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt No no it's totally fine! Truthfully I'm never sure and honestly go back and forth on this myself. I think it's worth trying to keep it standardised where possible. I work in a 120 col terminal though so this line length is totally fine.

This comment has been minimized.

@tofumatt

tofumatt Sep 19, 2018

Member

Indeed! I actually think we should have an editorconfig that makes explicit the tab width. The WordPress coding standards are quite deviant compared to most other JS projects, so we should make it easier for others to see how we work.

@tofumatt

tofumatt Sep 19, 2018

Member

Indeed! I actually think we should have an editorconfig that makes explicit the tab width. The WordPress coding standards are quite deviant compared to most other JS projects, so we should make it easier for others to see how we work.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Sep 19, 2018

Member

Okay, I just saw your explanation and now I understand this better. My apologies for not grokking it the first time!

Tested locally, including in IE 11, and it all seems good. I say :shipit:

Member

tofumatt commented Sep 19, 2018

Okay, I just saw your explanation and now I understand this better. My apologies for not grokking it the first time!

Tested locally, including in IE 11, and it all seems good. I say :shipit:

@chrisvanpatten

This comment has been minimized.

Show comment
Hide comment
@chrisvanpatten

chrisvanpatten Sep 19, 2018

Contributor

@tofumatt No worries, I probably should have included more of the context from #9263. Thanks for the review!

Contributor

chrisvanpatten commented Sep 19, 2018

@tofumatt No worries, I probably should have included more of the context from #9263. Thanks for the review!

@chrisvanpatten chrisvanpatten merged commit 9cc0a9a into WordPress:master Sep 20, 2018

2 checks passed

codecov/project 48.68% (-0.37%) compared to 546b744
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@chrisvanpatten chrisvanpatten deleted the TomodomoCo:try/simpler-icon-sizing branch Sep 20, 2018

jasmussen added a commit that referenced this pull request Oct 23, 2018

Fix icon size regression
This PR fixes an icon size regression that was introduced in #9828. Basically SVG icons are _between 20 and 24px_.

If an SVG has an explicit width, it uses that width, within the boundaries of those extremes. But unfortunately it also means if the SVG _doesn't_ have an explicit width or height, it collapses to the min-width and min-height.

This PR updates documentation, and adds explicit dimensions for all block icons used.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment