Page MenuHomePhabricator

Use a clearly intended height on MinervaNeue header
Closed, ResolvedPublic

Description

As of current, MinervaNeue's header height, which has been a result of some calculation 5 years ago, is set to 3.35em.
Which results in browser default settings in 53.6px.

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

Developer notes

As well as changing @headerHeight we'll need to update any calculations that use it (as LESS cannot deal with calculations of mixed units). Luckily there is only one case in the search overlay: top: ( @headerHeight / 2 ) - ( @iconSize / 2 ); That appears to be easy to deal with.

Event Timeline

Volker_E created this task.May 28 2018, 8:20 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 28 2018, 8:20 PM
Volker_E renamed this task from Use a clear intended height on MinveraNeue header to Use a clearly intended height on MinveraNeue header.May 28 2018, 8:20 PM
Jdlrobson renamed this task from Use a clearly intended height on MinveraNeue header to Use a clearly intended height on MinervaNeue header.May 29 2018, 1:32 PM

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.

Vvjjkkii renamed this task from Use a clearly intended height on MinervaNeue header to k4baaaaaaa.Jul 1 2018, 1:07 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
CommunityTechBot renamed this task from k4baaaaaaa to Use a clearly intended height on MinervaNeue header.Jul 2 2018, 3:31 PM
CommunityTechBot raised the priority of this task from High to Needs Triage.
CommunityTechBot updated the task description. (Show Details)
CommunityTechBot added a subscriber: Aklapper.
ovasileva triaged this task as Normal priority.Sep 17 2018, 4:51 PM
Jdlrobson set the point value for this task to 2.Sep 18 2018, 5:55 PM
Jdlrobson added subscribers: nray, Jdrewniak.

@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]

  1. 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.

Jdlrobson removed the point value for this task.Sep 25 2018, 6:27 PM

Will need re-estimation

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 Readers-Web-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.

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

Jdlrobson updated the task description. (Show Details)Jul 9 2019, 4:52 PM

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

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.

alexhollender closed this task as Resolved.Jul 31 2019, 12:57 PM
alexhollender removed alexhollender as the assignee of this task.

thanks for clarifying @matmarex