material icon indicating copy to clipboard operation
material copied to clipboard

menu: icon and label are slightly misaligned

Open AdriVanHoudt opened this issue 5 years ago • 7 comments

Bug, enhancement request, or proposal:

bug

CodePen and steps to reproduce the issue:

CodePen Demo which demonstrates the issue:

Detailed Reproduction Steps:

  1. go to https://material.angularjs.org/HEAD/demo/menu
  2. open menu and check icon alignment image

What is the expected behavior?

the icon aligns with the text

What is the current behavior?

it doesn't

What is the use-case or motivation for changing an existing behavior?

Which versions of AngularJS, Material, OS, and browsers are affected?

  • AngularJS: 1.7.x
  • AngularJS Material: 1.1.10
  • OS: macOS
  • Browsers: chrome / firefox

Is there anything else we should know? Stack Traces, Screenshots, etc.

This is a follow up of https://github.com/angular/material/issues/8464 The Firefox issue has been fix and afaik reverting the fix made for that issue fixes the alignment again 🎉 If you want I can do a PR for this

AdriVanHoudt avatar Nov 20 '18 15:11 AdriVanHoudt

Changing from inline-block to

    display: flex;
    align-items: center;
    align-content: flex-start;

Does seem to align the text slightly better.

What other parts of the previous fix do you think need to be reverted? What effect does that have?

A PR would be nice to have, but it will probably need to be targeted at the 1.2.0 release since it will break screenshot tests.

Splaktar avatar Nov 21 '18 00:11 Splaktar

Probably also need to revert the fixes that were added https://github.com/profoundry-us/material/commit/b294af2bc6f6bedc65b929cc20f3a76ff1090bbc#diff-ff328e798f1dc3e8a2f4dff8e0d639c5R89 But that needs to be checked. What does target 1.2.0 entail? Do I branch of a specific tag or?

AdriVanHoudt avatar Nov 21 '18 08:11 AdriVanHoudt

You'll just need to branch off of master and target master. It just means that it likely won't get merged for a couple of months.

Splaktar avatar Nov 21 '18 09:11 Splaktar

I messed with reverting some of those other parts and they appeared to still be needed.

Splaktar avatar Nov 21 '18 09:11 Splaktar

2 things:

  • The picture has a span, but span is no longer used in v1.1.10, so I'm not sure what version this is. There's no included CodePen

  • The screenshot comparsion is based on Firefox's inspection tool, which isn't an accurate assessment of orientation. Measuring the same screenshot manually, it seems alignment seems to actually be fine.

screen shot 2018-11-22 at 4 50 30 pm

If anything, there might be a pixel or two of misalignment, but you'll never be able to 100% fix that when it comes to font rendering. You could play around with line-height to shift it a little bit, but the improvements aren't much.

Edit: Here's another sample using CSS (and not inspection) to show the boundaries

button.md-button::before {
    content: "";
    top: 50%;
    width: 100%;
    position: absolute;
    height: 24px;
    border-top: solid 1px cyan;
    margin-top: -12px;
    border-bottom: solid 1px cyan;
}
screen shot 2018-11-22 at 4 57 28 pm

clshortfuse avatar Nov 22 '18 21:11 clshortfuse

It could be that I added the span to be able to draw those lines, if I did it didn't have a visual impact at the time. The screenshot was made in Chrome dev tools. The manual lines make it even more obvious for me as the middle red line is obviously not in the vertical middle of the text. And yes this is about a pixel or 2, that was the whole point :D and I know that seems minor but we and our clients notice this.

As to the release schedule, why don't they happen more often? Seems like more smaller releases would be way handier. I won't go into details but updating angular-material on our project has always been a pita.

AdriVanHoudt avatar Nov 23 '18 09:11 AdriVanHoudt

@AdriVanHoudt we're trying to do them more often, but 1.1.11 has been blocked for some time now by the need to get some P1 autocomplete and chips a11y fixes in. Those have been having trouble getting through the Google presubmit and sync process for the last couple of months. I've continued to work with the caretakers to get this done and we almost had a release last week after getting everything merged. Unfortunately, there were some minor breakages, so part of that had to be reverted and another sync process started. Unfortunately, it wasn't able to be completed before Thanksgiving.

I posted about this 1.1.11-rc.1 last week. Hopefully we'll get it shipped next week and the next release should be quicker. If possible, please try out the versions listed there and let me know if you find any issues.

Splaktar avatar Nov 23 '18 17:11 Splaktar