Page MenuHomePhabricator

[EPIC] Refactor icon sizing in MF/Minerva to achieve consistency
Open, NormalPublic8 Story Points

Description

Description

Currently there is not a consistent approach to icon sizing in MF/Minerva. There are generally three icon sizes (small, medium, and large) however the CSS that sizes individual icons is rather inconsistent. There is also inconsistency around the touch areas for icons. This problem became evident as we introduced several new icons into the interface as part of the AMC work. Our plan is to refactor the icon sizing, standardized around the following sizes:

smallmediumlarger than medium
icon area16x16px20x20px(no standard)
touch area34x34px44x44px(no standard)
example
see T231683

This problem is made worse by the fact that the SVGs themselves are not sized consistently. Some are on 24x24px canvases, others are on 20x20px canvases, with varying degrees of padding within the SVGs themselves (not CSS padding/margins). The work related to replacing icons with properly sized ones will be tracked in T231613.

This is an incomplete reference to help clarify which icons should be sized small vs. medium:

Example of current issue

  • Icons in the site header and page-actions toolbar are larger than icons in the menus.
  • Various icons, such as "talk" and "special pages", are being rendered larger than intended.

Developer notes

There seem to be two main causes of this issue:

  • The svgs themselves are not sized consistently. Some are on 24x24px canvases, others are on 20x20px canvases, with varying degrees of padding within the SVGs themselves (not CSS padding/margins)
  • The background-size: 100% auto; rule causes the width of the icons to 100%, which makes them appear larger if the SVG has a width smaller than 20px. See gif below.


QA

Run an exploratory test on https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain - if any content is missing or looks different to production and the UI element involves an icon flag it.

Some examples we have found so far: T232800, T232792, T232798

Sign off steps

  • Make sure all subtasks are resolved.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson renamed this task from Refactor icon sizing in MF/Minerva to achieve consistency to [EPIC] Refactor icon sizing in MF/Minerva to achieve consistency.Fri, Aug 30, 5:39 PM
Jdlrobson updated the task description. (Show Details)

Change 533586 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Separate text element from mw-ui-icon-before

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

Notes from our recent conversation:

  • It doesn't make sense to create a "large" standard icon size currently (see T231683)
  • We might want to be able to configure icons so they are flush-aligned but will defer that configurability for now
  • We discovered additional icons that need updated SVGs. Those SVGs will be added to T231613

Suggested course of action:

Phase 1 - unbreaking changes

  1. Patch needed to remove mw-ui-icon-large (n T231683)

Assignee: ??
Reviewer: ??

  1. Separate text from icon e.g. in main menu: https://gerrit.wikimedia.org/r/#/q/I3f803ec4c9068b30aa93b803391aa4d65d8310ff

Assignee: @Jdlrobson
Reviewer: @Jdrewniak

Phase 2 - remove the hacks

  1. Remove Hacks in MF: https://gerrit.wikimedia.org/r/533111
  2. Remove hacks in Minerva: https://gerrit.wikimedia.org/r/531558

Assignee: @Jdlrobson
Reviewer: @Jdrewniak

These changes should mostly be okay but might lead to a few misaligned icons until...

Phase 3 - new mw-ui-icon spec

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/533360/
Assignee: @Jdrewniak
Reviewer: @Jdlrobson

Preferably we'll want to merge this after a train cut to allow time for fixes.

Phase 4 - cleanup

Address any issues that come up in QA.

☝️I agree with this plan of action :)

@alexhollender, @Jdlrobson regarding the icon spec noted above (T229440#5452894), I've sketched out an implementation that I think meets the requirements and (hopefully) is compatible with the current CSS classes we're using. This implementation affords for small/medium icons with/without padding, as well as with padding flush left/right. In the instance where the icons are flush, the glyph sits at the edge of the touch area. @alexhollender you mention this in your last point, but I think overflowing the container is a bit risky as it could cause horizontal scrolling.

https://codepen.io/j4n/pen/VwZrzvy

ovasileva set the point value for this task to 8.Wed, Sep 4, 5:13 PM
Jdlrobson updated the task description. (Show Details)Wed, Sep 4, 7:08 PM

@Jdrewniak I checked out the codepen, looks great.

Change 533586 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Separate text element from mw-ui-icon-before

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

Change 533111 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove all the icon hacks

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

Change 531558 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Remove the mw-ui-icon hacks and overrides

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

Next in line:

Tomorrow (Tuesday):

Change 535655 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Also set bottom attribute on icon glyphs

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

Change 535655 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Also set bottom attribute on icon glyphs

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

Change 531558 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove the mw-ui-icon hacks and overrides

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

Change 533111 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove all the icon hacks

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

Update, all hacks have been removed and you will notice issues with certain icons and the resurfacing of bugs on the beta cluster (such as the issue in T230232)

Our focus now is updating the MediaWiki UI icon definition so icons are 20x20:
https://gerrit.wikimedia.org/r/533360 (Minerva (new icon definition))

@Jdrewniak and I are targeting at the very least getting this on reading web staging tomorrow for testing.

Change 533360 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] [WIP] Refactor mw-ui-icon for Minerva

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

Change 535835 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/extensions/MobileFrontend@master] [WIP] Remove uneccessary icon CSS after icon refactor

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

Change 535967 had a related patch set uploaded (by Jdlrobson; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Prepare for new mw-ui-icon spec for Minerva

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

Change 535970 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Update storybook to use new mw-ui-icon definition

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

Change 536174 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Utilize the mw-ui-icon-flush-left/right classes to align icons

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

Change 536312 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Prepare for the icon changes

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

Jdlrobson added a comment.EditedThu, Sep 12, 6:49 PM

Cross browser testing results

On iOS the search and hamburger feel a little tight to the sides:

I think apart from this we're good to go.

Change 535967 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Prepare for new mw-ui-icon spec for Minerva

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

Change 536312 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Prepare for the icon changes

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

Change 533360 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] New mw-ui-icon spec for Minerva

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

Jdlrobson updated the task description. (Show Details)Fri, Sep 13, 12:00 AM

Change 535970 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Update storybook to use new mw-ui-icon definition

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

Section collapse toggles look wrong when the collapsible section is a <h1> or <h3> instead of <h2>, as normal.

This seems to be caused by this block in Toggler.less: https://phabricator.wikimedia.org/diffusion/EMFR/browse/master/resources/mobile.startup/Toggler.less$5

However, when I tried removing it, the styling for collapsible sections in other mobile skins got all messed up.

I'll leave it for you to untangle…

Change 535835 merged by Jdrewniak:
[mediawiki/extensions/MobileFrontend@master] Remove unneccessary icon CSS after icon refactor

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

@Jdlrobson @Jdrewniak here are the issues I'm seeing on beta:

Main site header
hamburger and search are too close to the edges
Expand/collapse icon
too far over to the leftnot vertically centered with section heading
Overlays (talk, notifications, search)

The "X" has too much padding, which is causing two issues:

it should be closer to the edge of the screenit's pushing the element next to it too far over
Main menu
alignment issue, text slightly too high for some menu items
Search within page element
alignment issue, should be further to the right
Log-out icon
Bytes changed icon
alignment issue, should be further to the left
Citation drawer
alignment issues, outer elements should be closer to edge of pagegeneral horizontal centering issues

Other issues, not sure if they are related to icon changes

close modal icon alignment issuepublish button clipped
add discussion button on talk overlay
last edited bar issues...
thank button on diff page

Other issues, not sure if they are related to icon changes

close modal icon alignment issuepublish button clipped

Close text appearing tracked in T232798

last edited bar issues...

tracked in T232800

Will probably have to open bugs for the rest

I found a weird issue on Firefox (on production) - The logout icon is misaligned -

Change 536618 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Flush reference drawer and Special:MobileDiff icons to relevant places

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

Change 536624 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Search content margin should match

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

Change 536626 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Drop width declaration on close button

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

Change 536624 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Search content margin should match search input

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

Change 536618 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Flush reference drawer and Special:MobileDiff icons to relevant places

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

@Jdlrobson @Jdrewniak here are the issues I'm seeing on beta:

Main site header
hamburger and search are too close to the edges

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/536174/

Expand/collapse icon
too far over to the leftnot vertically centered with section heading

https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/536174/

Overlays (talk, notifications, search)

The "X" has too much padding, which is causing two issues:

it should be closer to the edge of the screenit's pushing the element next to it too far over

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

Main menu
alignment issue, text slightly too high for some menu items

STILL NEEDS A FIX

Search within page element
alignment issue, should be further to the right

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

Log-out icon

tracked in T232792

Bytes changed icon
alignment issue, should be further to the left

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

Citation drawer
alignment issues, outer elements should be closer to edge of pagegeneral horizontal centering issues

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

Other issues, not sure if they are related to icon changes

close modal icon alignment issuepublish button clipped

tracked in T232798

add discussion button on talk overlay

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

last edited bar issues...

tracked in T232792

thank button on diff page

NEEDS FIX

Change 536661 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Fix Main Menu text alignment problems

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

After Fix thanks extension works as expected

Change 536660 had a related patch set uploaded (by Pmiazga; owner: Jdlrobson):
[mediawiki/extensions/Thanks@master] Create mobile thanks using the mobile library

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

Change 536661 merged by Jdlrobson:
[mediawiki/skins/MinervaNeue@master] Fix Main Menu text alignment problems

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

Change 536626 merged by Jdlrobson:
[mediawiki/skins/MinervaNeue@master] Drop width declaration on close button

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

Change 536174 merged by Jdlrobson:
[mediawiki/skins/MinervaNeue@master] Utilize the mw-ui-icon-flush-left/right classes to align icons

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

Change 536660 merged by Jdlrobson:
[mediawiki/extensions/Thanks@master] Create mobile thanks using the mobile library

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