fix(mdButton): icon vertical alignment

[epelc]

I applied the display change to .md-button md-icon because regular buttons with icons and fab buttons also require this fix.

This fixes md-icon vertical alignment within md-button. Closes #8066

[ThomasBurleson]

@devversion - can you review/test ?

[epelc]

See the notes on the issue for how to test it easily.

On Sunday, April 24, 2016, Thomas Burleson [email protected] wrote:

@devversion https:/DevVersion - can you review/test ?

—
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#8194 (comment)

Best regards,
Ed Pelc
President

Website: freightchick.com
Phone: 941-735-4047

[devversion]

I think this comment is actually not saying really much.

Can we explain more, why this is needed and for example, which components are affected by the vertical alignment problem?

[epelc]

@devversion Agreed. I'll see if I can figure out why it actually fixes it. iirc layout="row" has a rule to have all children use display: block;.

[epelc]

So I did some digging, and the icon alignment issue was actually introduced way back in 0.10.1 by the fix for #3383. That fix changed .md-button's display property from flex to inline-block. Which made .md-button's align-items: center; a no op because it requires a flex container. This causes the icon to fallback to default browser alignment for buttons.

The default browser alignment for buttons doesn't appear to use any css properties and is just something specific to buttons. I couldn't find any info about this around the web or in the spec for buttons. It's using some hidden builtin method for vertical alignment, instead of a regular css property.

I'll update my comment shortly.

[epelc]

Note I also just tested this with a href buttons and the fix works as expected.

[ThomasBurleson]

👍

[ivoviz]

I was curious since I've already opened an issue about this (#7257), so I've tested. It only works for fab buttons. Didn't you misplace the rule @epelc? Also, if you add this to .md-button md-icon as you mentioned in the first comment, it would break buttons with icon and text together.

[epelc]

@ivoviz In the original issue I was thinking to add it for .md-icon-button md-icon but then I switched to .md-button md-icon so that it also fixes fabs which do not use md-icon-button.

It does in fact break the layout in a couple cases. I original checked that a href works with an md-icon-button but it is broken with a .md-fab using an a href.

Here is a demo with all the combos of button/a href and the different classes.

[epelc]

Here is an updated demo that's working across all cases.

Using this rule

.md-button.md-icon-button,
button.md-button.md-fab {
  md-icon {
    display: block;
  }
}

I'll update my pr to include this.

[ivoviz]

@epelc Yup, this new one seems to work, thanks!

[devversion]

@epelc Just wanted to let you know, that I'll test the PR tomorrow.

[devversion]

I'm sorry that I didn't test it until now. Issues are sometimes getting lost.

I just checked your PR out and compared with the latest demos.
And the PR seems to be correct / fine.

Some calculations:

• Button Height: 56px
• Icon Height: 24px

So the correct top offset (offsetTop) should be (56px - 24px) / 2.

• Head: 17px
• PR: 16px (correct)

Since this is actually testable, we should add this as a unit test, because we're able to execute those calculations on Headless browsers as well.

Once the Test is there, LGTM.