Page MenuHomePhabricator

[Regression]: Remove extra spacing between header and page title (from empty site notice)
Closed, ResolvedPublic3 Estimated Story Points

Description

Background

The initial fix from T312749 introduced extra 24px spacing between the header and page title.

currentprevious/correct
image.png (1×2 px, 2 MB)
image.png (1×2 px, 1 MB)

Before
After

More context: https://phabricator.wikimedia.org/T312749#8163262

AC

  • There is a 24px spacing between the header and page title when the site notice is empty
  • When the site notice is present there is a 24 px spacing between the header, site notice, and page title

Developer notes

The simplest solution is to removing row gap from the grid container. This impacts the spacing of the TOC in all the non collapsed cases (i.e. main menu closed, main menu open, scrolled down with sticky header, scrolled down without sticky header), so the TOC CSS should be carefully updated and QAed.

Event Timeline

when looking at this I also noticed that the height of the site header is defined as 66px, rather than using min-content as all of the other grid rows do (aside from the main content row). from the standpoint of evaluating layout issues (like this one), and just generally being able to easily understand the front-end, it seems like it would be better if we used min-content and then added padding: 8px 0 to .mw-header.

currentupdated
image.png (1×3 px, 2 MB)
image.png (1×3 px, 2 MB)
alexhollender_WMF renamed this task from Remove extra spacing between header and page title from empty site notice to [Visual refinements] Remove extra spacing between header and page title from empty site notice.Aug 19 2022, 2:12 PM
Jdlrobson subscribed.

This particular bug is due to our use of row-gap and the site notice row being empty. A quick Google didn't yield any answers for me on how to avoid this, so I suspect solving this will involve removing the row gap and using margins instead.

ovasileva renamed this task from [Visual refinements] Remove extra spacing between header and page title from empty site notice to Regression: Remove extra spacing between header and page title from empty site notice.Aug 22 2022, 3:20 PM
ovasileva renamed this task from Regression: Remove extra spacing between header and page title from empty site notice to [Regression]: Remove extra spacing between header and page title from empty site notice.

We should backport this one once it's fixed

Change 825401 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Remove grid row gap in favor of margins

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

Change 825401 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Remove grid row gap in favor of margins

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

Change 825401 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Remove grid row gap in favor of margins

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

Change 825752 had a related patch set uploaded (by Jdlrobson; author: Bernard Wang):

[mediawiki/skins/Vector@wmf/1.39.0-wmf.25] Remove grid row gap in favor of margins

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

Next step: backporting and QA. Note backporting will also clean out the noise in https://pixel.wmcloud.org/reports/desktop/index.html.

It looks like this can't easily be backported without backporting another change. I'm getting variable @selector-main-menu-closed is undefined - do you know which patch introduces that element?

Change 825752 abandoned by Jdlrobson:

[mediawiki/skins/Vector@wmf/1.39.0-wmf.25] Remove grid row gap in favor of margins

Reason:

Can't be backported - requires another Vector patch to be backported first but I am not sure which.

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

alexhollender_WMF renamed this task from [Regression]: Remove extra spacing between header and page title from empty site notice to [Regression]: Remove extra spacing between header and page title (from empty site notice).Aug 23 2022, 4:45 PM

@Jdlrobson Looks like it was https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/824267, was a small CSS variable clean up patch so i believe it's not as risky relatively speaking. Hoping that's the only dependency

Change 825752 restored by Jdlrobson:

[mediawiki/skins/Vector@wmf/1.39.0-wmf.25] Remove grid row gap in favor of margins

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

Change 825889 had a related patch set uploaded (by Urbanecm; author: Bernard Wang):

[mediawiki/skins/Vector@wmf/1.39.0-wmf.26] Remove grid row gap in favor of margins

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

Change 825752 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.39.0-wmf.25] Remove grid row gap in favor of margins

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

Change 825889 merged by jenkins-bot:

[mediawiki/skins/Vector@wmf/1.39.0-wmf.26] Remove grid row gap in favor of margins

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

Change 826307 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Site notice spacing fix

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

Change 826355 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Remove grid row gap in favor of margins

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

Change 826355 abandoned by Bernard Wang:

[mediawiki/skins/Vector@master] Reapply "Remove grid row gap in favor of margins"

Reason:

squashed into https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/826307

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

Change 826307 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Fix site notice spacing

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

Change 827534 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@wmf/1.39.0-wmf.26] Fix site notice spacing

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

Change 827534 merged by Clare Ming:

[mediawiki/skins/Vector@wmf/1.39.0-wmf.26] Fix site notice spacing

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

Mentioned in SAL (#wikimedia-operations) [2022-08-29T20:42:29Z] <cjming@deploy1002> Synchronized php-1.39.0-wmf.26/skins/Vector: Backport: [[gerrit:827534|Fix site notice spacing (T315595)]] (duration: 03m 46s)

Jdlrobson added a subscriber: Jdrewniak.

Skipping QA in beta cluster because this was backported.

Test Result - Prod

Status: ✅ PASS
Environment: testwiki
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: There is a 24px spacing between the header and page title when the site notice is empty

Screen Shot 2022-08-31 at 6.29.09 PM.png (367×1 px, 158 KB)

✅ AC2: When the site notice is present there is a 24 px spacing between the header, site notice, and page title

Screen Shot 2022-08-31 at 6.28.00 PM.png (789×1 px, 318 KB)

Change 829046 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Followup: clean up cached HTML code from T315595

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

Change 829046 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Followup: clean up cached HTML code from T315595

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

Change 829046 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Followup: clean up cached HTML code from T315595

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