Page MenuHomePhabricator

[Minerva] Increase space between icon and text label in main menu
Closed, ResolvedPublic3 Estimated Story Points

Description

Description

Somewhere along the way the spacing between the icon and text label in the main menu seems to have decreased. This seems to be the case for both AMC and non-AMC:

Screen Shot 2021-10-04 at 11.56.24 AM.png (904×951 px, 66 KB)

reference to the previous/correct version: https://web.archive.org/web/20191101063058/https://en.m.wikipedia.org/wiki/Main_Page

Per T292452#7400318 for consistency with Vector this should be an 8px margin.

Developer notes

The change should be done in core inside the mw-ui-icon library. Any no longer needed rules inside Vector should be removed.

QA Results - Beta

ACStatusDetails
1T292452#7555081

QA Results - Prod

ACStatusDetails
1T292452#7555082

Event Timeline

Restricted Application added a subscriber: Masumrezarock100. · View Herald Transcript

Both Vector and Minerva are converging on the same icon implementation as part of the user menu feature, so this regression looks like it came about then.

Should be straightforward to fix, but the margin to the right of the icon in Vector here is 8px:

Screen Shot 2021-10-04 at 2.06.30 PM.png (94×466 px, 4 KB)

Should this also become 12px ?

@Jdlrobson thanks for calling that out. I think the current Vector implementation is correct so lets match that. cc @Volker_E

It looks like the 8px right margin in Vector is due to mw-ui-icon-before, while Minerva only uses mw-ui-icon and an extra whitespace. I'm not sure if it makes sense to make changes to the core mw-ui-icon library, as mw-ui-icon isn't supposed to have padding or margin . Perhaps it makes more sense to just update Minerva to use mw-ui-icon-before like Vector, or to just add some extra CSS to apply the right margin?

I think that would be fine @bwang

We could however use the next sibling selector in this case: mw-ui-icon + span { margin-left: 8px; } if that made sense as we'd like to move away from mw-ui-icon-before

Change 736073 had a related patch set uploaded (by Clare Ming; author: Clare Ming):

[mediawiki/skins/MinervaNeue@master] Fix spacing between icon + text in main menu

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

cjming added a subscriber: cjming.

Please see requested follow up work to make this part of core.

Change 736073 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Fix spacing between icon + text in main menu

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

Change 736919 had a related patch set uploaded (by Clare Ming; author: Clare Ming):

[mediawiki/core@master] Update margin rule for menu icons.

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

Change 737448 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Partial revert \"Fix spacing between icon + text in main menu\"

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

Change 736919 merged by jenkins-bot:

[mediawiki/core@master] Update margin rule for menu icons.

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

Change 737448 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Partial revert \"Fix spacing between icon + text in main menu\"

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

@Jdlrobson, I'm not sure if this is the correct way to validate it, but, here's how it looks in the web archive:

Screen Shot 2021-12-06 at 6.29.27 AM.png (459×890 px, 121 KB)

That shows the 12px. Beta and Prod show 8px.

Screen Shot 2021-12-06 at 6.31.21 AM.png (460×890 px, 121 KB)

Screen Shot 2021-12-06 at 6.32.36 AM.png (459×886 px, 115 KB)

@Edtadros apologies for the confusion, we settled on 8px, so that's correct. I just never went back and updated the mock.

Test Result - Beta

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

Test Artifact(s):

QA Steps

✅ AC1: Spacing between icon and text label should be 8px (see: T292452#7549913)

Screen Shot 2021-12-06 at 6.31.21 AM.png (460×890 px, 121 KB)

Test Result - Prod

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

Test Artifact(s):

QA Steps

✅ AC1: Spacing between icon and text label should be 8px (see: T292452#7549913)

Screen Shot 2021-12-06 at 6.32.36 AM.png (459×886 px, 115 KB)

thanks @alexhollender and @Jdlrobson !