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: Disabled block switcher icons are blurry #15643

Merged
merged 1 commit into from
May 16, 2019

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 15, 2019

Description

Fixes #15629.

It looks like IconButton has a very unexpected logic for adding has-text class. It adds it when children prop is passed which was the case. However, an icon isn't really the text 🤷‍♂

Before

When the block switcher is disabled, the block icon inside of it appears smaller than it should, resulting in some blurry icons on non-retina screens. Here's the Container block icon for instance, appearing at 20px wide:

Screen Shot 2019-05-14 at 9 43 41 AM

For comparison, here's a non-disabled icon, which renders at the correct 24px wide:

Screen Shot 2019-05-14 at 9 47 24 AM

Screenshots

After

Screen Shot 2019-05-15 at 11 59 23

@gziolo gziolo added [Type] Bug An existing feature does not function as intended [Feature] Blocks Overall functionality of blocks labels May 15, 2019
@gziolo gziolo self-assigned this May 15, 2019
@gziolo gziolo requested review from jasmussen and kjellr May 15, 2019 10:00
@gziolo gziolo force-pushed the fix/block-no-switcher-icon branch from 6943126 to 3f1a8af Compare May 15, 2019 11:14
Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

This definitely fixes the issue. All the icons look perfectly crisp with this PR:

Screen Shot 2019-05-15 at 7 58 01 AM
Screen Shot 2019-05-15 at 7 58 10 AM
Screen Shot 2019-05-15 at 7 58 18 AM

I'll hold off for a developer's input on the actual method used. But I can confirm that this absolutely solves the problem. I don't see any negative effects to other block switcher icons either. Thanks, @gziolo!

@gziolo
Copy link
Member Author

gziolo commented May 16, 2019

I think that having both Travis and @kjellr happy is enough to proceed but I will wait for ✅ :)

@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label May 16, 2019
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Seems legit for me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabled block switcher icons are blurry
3 participants