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 blocks navigation menu SVG icon size. #11153

Merged
merged 6 commits into from
Oct 30, 2018

Conversation

afercia
Copy link
Contributor

@afercia afercia commented Oct 27, 2018

Fixes #11152
Fixes #11150

Adds a missing height to the Blocks navigation menu SVG icon. Fixes the misplacement of the related menu in Internet Explorer 11. See screenshot on #11152

Also exposes the keyboard shortcut in the button tooltip.

@afercia afercia requested a review from youknowriad October 27, 2018 14:22
@afercia afercia added [Type] Bug An existing feature does not function as intended Browser Issues Issues or PRs that are related to browser specific problems labels Oct 27, 2018
@youknowriad
Copy link
Contributor

I wonder if we should use the newly added Icon component inside the IconButton component (for consistency). Not sure what impact it would have but feels like the right thing to do/try.

@afercia
Copy link
Contributor Author

afercia commented Oct 27, 2018

@youknowriad
Copy link
Contributor

@afercia I pushed the change I proposed which is more generic but also more impacting. So I'm not certain yet at this point.

The idea is to use Icon component as the canonical component to render our icons (used by IconButton in this case).

@jasmussen @aduth Thoughts?

This also can impact all IconButtons :) In my testing, it looks good though.

@jasmussen
Copy link
Contributor

jasmussen commented Oct 29, 2018

Nice PR.

There's one small issue though — the button is too tall. Compare:

screenshot 2018-10-29 at 12 24 08

screenshot 2018-10-29 at 12 28 36

For whatever reason the height attribute of the icon itself is 24, when the width is 20:

screenshot 2018-10-29 at 12 24 29

I suspect this is partially because this icon was designed on a 24x24px grid, and the remaining editor bar icons are still on the 20x20 grid. Long term more icons are likely to be on the 24x24 grid, but in this case it's a mix.

This mix is also okay, because the 24x24px icon still looks crisp while scaled down to 20x20 (matching the other icons). So in this case, the solution is to make sure both the width and height of the icon are 20, even though the viewbox remains 0 0 24 24.

@youknowriad
Copy link
Contributor

The height should be fixed.

@jasmussen
Copy link
Contributor

Thank you! All good from my end.

@afercia
Copy link
Contributor Author

afercia commented Oct 29, 2018

As long as a viewBox is applied to all the rendered <svg> icons together with proper width and height I guess it should be fine on all browsers. Sorry I don't have so much time to check in depth, thanks for the ping.

@afercia
Copy link
Contributor Author

afercia commented Oct 29, 2018

@youknowriad if you think it's good to go, please do feel free to merge it. I'm afraid I don't have time to dedicate today and in the next days. Thanks!

@youknowriad youknowriad added this to the 4.2 milestone Oct 30, 2018
@youknowriad youknowriad merged commit c6a1b18 into master Oct 30, 2018
@youknowriad youknowriad deleted the fix/blocks-navigation-menu-svg-icon-size branch October 30, 2018 07:50
@designsimply designsimply added the [Feature] List View Menu item in the top toolbar to select blocks from a list of links. label Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Browser Issues Issues or PRs that are related to browser specific problems [Feature] List View Menu item in the top toolbar to select blocks from a list of links. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants