cc @Jdlrobson @alexhollender

I believe the header height is related to icons (@iconSize: 1.5em + @iconSize: 1.5em; + 0.35em (padding top/bottom of forms in mediawiki.ui/components/forms.less), I'm not sure how the calculation here was made. This would be a more logical calculation if we continued to use it... :) however we'd need to switch from ems to pixels for the iconSize.

Sounds good to me.

@pmiazga did an async estimation - @nray and @Jdrewniak raised some interesting concerns:

Nick Ray [14 minutes ago]

1. Because em is influenced by the browser’s font size setting, if we change to px, the header will not look correct when someone changes their browser > front size from the standard. Do / should we care about that? From what I recall, a lot of styles were based on em, not px. (edited)

Nick Ray [13 minutes ago]

2. I didn’t really understand the motivation behind this change. What problems is the current approach causing?

Jan Drewniak
I gotta reply on this ticket. I would hope that the height of the header leaves enough room for the things inside the header, so I’m not sure why we need to define the height at all
I think we can get rid of that 5 year old fixme in the code though 😛

^ just to elaborate on my thoughts there...
My feeling was that the items inside the header should define its height, and that a statically defined height might break when the items inside are resized or changed.
Looking at the code though, I noticed the header is defined with display: table-cell. This property actually ensures that the height is never smaller than the contents inside it (see here for example video) so, that's pretty cool.

I still think using some sort of calculation is preferable to a static number though, because the header is meant to accommodate icons, so having the height change based on a defined icon size still seems like a good idea to me.

Tracking down where the headerHeight actually comes from, it looks like it's just statically defined as 3.35em.

Moving to backlog for more analysis!

@Volker_E thoughts?

Will need re-estimation

In T195795#4611240, @Jdrewniak wrote:

Tracking down where the headerHeight actually comes from, it looks like it's just statically defined as 3.35em.

So 3.35em * 16px = 53.6px.
3.375 * 16px = 54px

There doesn't seem to be anything stopping us from redefining headerHeight to 3.375em, but it's not exactly clear what problem this solves, given browser font size can change... please revise and throw back to Web-Team-Backlog

The problem it solves is uneven distance when vertically centering something at the default font-size. It's a visual glitch.
We don't necessarily have to care about user font-size choices besides the default, but when unchanged the UI should present itself as intended.
Side-note: When taking into account user font size choices, the best way is to use min-height over height where applicable.

@Jdrewniak, @nray thoughts?

Change 521543 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Set header height to fixed pixel

https://gerrit.wikimedia.org/r/521543

Related: (and my pet peeve) T215426: Consider changing mobile site header height (and overlay header height) from 3.35em (53.6px) to 3em (48px)

Changing it to 48px is a little trickier as it requires adjusting more calculations, but should be easier once this is done first.

It turns out this bug is the same as T225260

Change 521543 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Set header height to fixed pixel

https://gerrit.wikimedia.org/r/521543

Change 524250 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Set header height to 54px equivalent in ems

https://gerrit.wikimedia.org/r/524250

Change 524250 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Set header height to 54px equivalent in ems

https://gerrit.wikimedia.org/r/524250

@alexhollender can you confirm this is done and move to sign off?

checked through a number of headers, everything looks normal to me. @Volker_E in the description it says:

Let's set this to a full pixel equivalent, proposed 54px

but it shows at 55px when I inspect in, with the height defined as 3.375em

Is that what's intended?

The inspector shows 55px because it counts the border-top: 1px, which is actually made invisible my margin-top: -1px (the border is apparently shown when a CentralNotice banner is shown, or something). It is, in fact, 54px tall.