Skip to content
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

Fix icon size regression #10938

Closed
wants to merge 1 commit into from
Closed

Fix icon size regression #10938

wants to merge 1 commit into from

Conversation

jasmussen
Copy link
Contributor

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.

Before:

screen shot 2018-10-23 at 11 34 25

After:

screen shot 2018-10-23 at 11 39 07

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.
@jasmussen jasmussen added the [Type] Regression Related to a regression in the latest release label Oct 23, 2018
@jasmussen jasmussen added this to the 4.2 milestone Oct 23, 2018
@jasmussen jasmussen self-assigned this Oct 23, 2018
@jasmussen jasmussen requested review from chrisvanpatten and a team October 23, 2018 09:53
@chrisvanpatten
Copy link
Contributor

@jasmussen I think this is actually another issue. In the BlockIcon component, we should be enforcing width="24" and height="24" if those attributes aren't provided:

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-icon/index.js#L24-L25

I'm inclined to think this is related to all the React Native-related SVG refactoring… cc @gziolo

That said I think it's good for this to land either way — better to be explicit about width/height if possible because it encourages users to do the same!

Copy link
Contributor

@chrisvanpatten chrisvanpatten left a comment

Choose a reason for hiding this comment

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

Approving for the explicitness of providing width/height (I think it's good for us to be explicit) but we should still look into why the default width/height attributes aren't being applied inside <BlockIcon />.

@jasmussen
Copy link
Contributor Author

In the BlockIcon component, we should be enforcing width="24" and height="24" if those attributes aren't provided:

I really like the sound of that, and I'd like to do that in this branch. Let me see if I can.

@jasmussen
Copy link
Contributor Author

Boy that's a gnarly rebase. Something big just changed with embeds that moved it all elsewhere. I might do a complete re-do of this one. Apologies if that requires an extra ping.

@jasmussen
Copy link
Contributor Author

Interesting, from a casual dive into the BlockIcon component, it looks like it should, as Chris suggests, output explicit widths and heights on each icon, according to https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-icon/index.js#L25. @gziolo you are stronger than me in code, can you figure out why those attributes aren't properly output?

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Oct 23, 2018

@jasmussen Sorry yes that was slightly confusingly worded — I meant that it should already be enforcing that 24 default 😄

There was a lot of work done around the move to the React Native-friendly version of the SVG component so my guess is that it's connected to that work.

Also instead of a rebase you could probably just do git checkout master packages/block-library/src/embed/index.js which should bring the master version of the embed file back into this branch, and then you can commit this and open a new PR for the embeds.

jasmussen pushed a commit that referenced this pull request Oct 24, 2018
This is an alternative to #10938 which for whatever reason I can't rebase. The purpose is the same:

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.

Before:

![screen shot 2018-10-23 at 11 34 25](https://user-images.githubusercontent.com/1204802/47352448-32d6d000-d6ba-11e8-8ab8-c83fea8e8b07.png)

After:

![screen shot 2018-10-23 at 11 39 07](https://user-images.githubusercontent.com/1204802/47352456-35392a00-d6ba-11e8-832e-a45f3772389c.png)
@jasmussen
Copy link
Contributor Author

Closing this in favor of #10987.

@jasmussen jasmussen closed this Oct 24, 2018
@jasmussen jasmussen deleted the fix/icon-size-regression branch October 24, 2018 08:32
jasmussen added a commit that referenced this pull request Oct 24, 2018
* Fix icon size regression.

This is an alternative to #10938 which for whatever reason I can't rebase. The purpose is the same:

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.

Before:

![screen shot 2018-10-23 at 11 34 25](https://user-images.githubusercontent.com/1204802/47352448-32d6d000-d6ba-11e8-8ab8-c83fea8e8b07.png)

After:

![screen shot 2018-10-23 at 11 39 07](https://user-images.githubusercontent.com/1204802/47352456-35392a00-d6ba-11e8-832e-a45f3772389c.png)

* Pretty up the code a bit.
antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018
* Fix icon size regression.

This is an alternative to WordPress#10938 which for whatever reason I can't rebase. The purpose is the same:

This PR fixes an icon size regression that was introduced in WordPress#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.

Before:

![screen shot 2018-10-23 at 11 34 25](https://user-images.githubusercontent.com/1204802/47352448-32d6d000-d6ba-11e8-8ab8-c83fea8e8b07.png)

After:

![screen shot 2018-10-23 at 11 39 07](https://user-images.githubusercontent.com/1204802/47352456-35392a00-d6ba-11e8-832e-a45f3772389c.png)

* Pretty up the code a bit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants