Page MenuHomePhabricator

It should be possible to customise the implementation of mw.util.addPortletLink function on a skin basis
Closed, ResolvedPublic

Description

mw.util.addPortletLink works inside Minerva but it produces menu items that have no design consistency with the rest of the skin.

Screenshot 2019-10-28 at 10.26.15 AM.png (391×344 px, 24 KB)

Screenshot 2019-10-28 at 10.25.38 AM.png (211×319 px, 10 KB)

Screenshot 2019-10-28 at 10.28.12 AM.png (392×295 px, 25 KB)

While gadget developers can update their associated styles to make their items look like the others, such a solution is not going to be resilient to changes.

In T231925 https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/545636/7/resources/src/mediawiki.util/util.js was proposed as the way to resolve this - for compatiblity with Minerva, text would be in a span and and assigned an icon. Ideally such code would live in Minerva.

Let's allow skins to define the render function for mw.util.addPortletLink.
How would this work in practice? Could ResourceLoaderSkinModule be used?

Developer notes

A portlet link is added by:

mw.util.addPortletLink('p-interaction', 'http://en.wikipedia.org/wiki/Wikipedia:Dashboard', 'Dashboard', 'n-dashboard', 'Visit the dashboard', 'd');

A monkey patch like so gives the right result and a programmatically created icon:

var _portletLink = mw.util.addPortletLink;
mw.util.addPortletLink = function( _1, _2, _3, id ) {
  var $item = $(_portletLink.apply(this,arguments));
  var $a = $item.find('a');
  $a.addClass('menu-list-item__button menu__item--' + id + ' mw-ui-icon mw-ui-icon-before mw-ui-icon-portletlink-' + id)
  var $label = $('<span class="menu-list-item__label">').text( $item.text());
  $a.empty().append($label);
  return $item[0];
}

Instead of monkey patching addPortletLink could fire a hook allowing modification of menu items. mw.hook('addPortletLink').fire and Minerva should subscribe to that hook using mw.hook('addPortletLink').add

Event Timeline

Important to think about from an architectural standpoint if we rethink menus in the desktop improvement project.

Change 567439 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/MinervaNeue@master] Style links added by mw.util.addPortletLink() consistent with basic menu items: use css selectors starting at the list root, traversing element tags (consistent with other skins).

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

Why not do with pure "declarative" css?

minerva-menu-fix-user.png (342×284 px, 11 KB)
minerva-menu-fix-nav.png (432×392 px, 13 KB)
minerva-menu-fix-more.png (362×276 px, 12 KB)

Test with this: https://userstyles.org/styles/179583/minerva-user-menu-addportletlink-fix

The second "Preferences" link is a dummy portlet link.

Jdlrobson added subscribers: Jdrewniak, Niedzielski.

Moving into sprint board since there is a patch from a volunteer.

In T236711#5833986, @AronManning wrote:

Why not do with pure "declarative" css?

minerva-menu-fix-user.png (342×284 px, 11 KB)
minerva-menu-fix-nav.png (432×392 px, 13 KB)
minerva-menu-fix-more.png (362×276 px, 12 KB)

Test with this: https://userstyles.org/styles/179583/minerva-user-menu-addportletlink-fix

The second "Preferences" link is a dummy portlet link.

This still doesn't solve the icon problem so is not a complete solution IMO. A missing icon looks just as broken to me, maybe more broken?

I'm also not sure if we want to depart from the BEM CSS selectors we've been experimenting in Minerva. It reintroduces the problem of conflicting CSS rules, it makes maintenance harder and it certainly makes CSS less readable. Having two types of links (one with icons and one without) also means more combinations to test, so we should weigh up the pros and cons.

cc @Jdrewniak and @Niedzielski can I request your perspective on this?

In T236711#5833986, @AronManning wrote:

This still doesn't solve the icon problem so is not a complete solution IMO.

Adding a default icon is possible, however only with the duplication of .mw-ui-icon.mw-ui-icon-before classes.
Or in javascript, by adding classes mw-ui-icon mw-ui-icon-before mw-ui-icon-minerva-?portlet? to the anchor element.
Adding an icon wasn't my target: it's just gadgets. At least the font and color are consistent.

A missing icon looks just as broken to me, maybe more broken?

The css would be simpler without indent. I've tried that first, both looks broken imo :D It also looked unorganized, so I went with indentation. Subjective, both possible.

minerva-menu-fix-noicon-short.png (306×247 px, 10 KB)
minerva-menu-fix-noicon-long.png (309×258 px, 11 KB)
minerva-menu-fix-indent-long.png (311×258 px, 11 KB)
minerva-menu-fix-indent-long-nav.png (224×329 px, 6 KB)

I'm also not sure if we want to depart from the BEM CSS selectors we've been experimenting in Minerva.

Not all BEM is lost, but it's indeed less flat: .toggle-list-item__anchor was replaced with .toggle-list__list--drop-down li > *
toggle-list-item__anchor became unnecessary, a few hundred bytes can be saved in the html. I guess both has benefits.

Css/Js

Also note that some old/unmaintained gadgets add the element directly, without mw.util.addPortletLink(). Those will be fixed only with css.
The css is good for fixing the layout, the font and the color.
To add a default icon is more optimal with js (add 3 classes). The 8th parameter could be used to set the icon's name.
With pure css the .mw-ui-icon-before needs to be duplicated (ca. 10 rules, sizes need to be kept consistent - not a big issue).

Is there a portlet icon? I should add it for a demo.
Do you want to open the possibility for gadgets to set an icon in the 8th parameter, or a new function? Imho would be nice. The new Vector and Timeless could support it.

What I've found maybe usable from ooui icons:

40px-OOjs_UI_icon_puzzle-ltr-progressive.svg.png (40×40 px, 588 B)
40px-OOjs_UI_icon_link-ltr-progressive.svg.png (40×40 px, 854 B)
40px-OOjs_UI_icon_attachment-ltr-progressive.svg.png (40×40 px, 845 B)
40px-OOjs_UI_icon_pageSettings-progressive.svg.png (40×40 px, 829 B)
40px-OOjs_UI_icon_lightbulb.svg.png (40×40 px, 595 B)
40px-OOjs_UI_icon_advanced-progressive.svg.png (40×40 px, 1 KB)

Not all BEM is lost, but it's indeed less flat:

In BEM one of the fundamental rules is to never nest selectors. A selector .toggle-list__list--drop-down li is thus not BEM. So such a CSS change would need to avoid that confusion by not following BEM.

Is there a portlet icon? I should add it for a demo.

Portlet icons should be different for every menu link, with some kind of default where not specified.

Do you want to open the possibility for gadgets to set an icon in the 8th parameter, or a new function? Imho would be nice. The new Vector and Timeless could support it.

This is exactly what I was pushing for initially in T231925#5607253 and https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/545636/ - please see those for background.
Now I'm less sure and think a hook approach might be better.

I think the simplest solution would be JS only and work like this:

  1. mediawiki.util.addPortletLink fires a hook after constructing the menu item
mw.hook( 'mediawiki.util.addPortletLink'  ).fire( item )
  1. Minerva subscribes to the hook and massages the DOMElement - adding appropriate classes and icon based on the id of the element e.g. if portlet id is ca-move - it creates an icon `mw-ui-icon-ca-move. This code lives in skins.minerva.scripts (Assuming that will load BEFORE gadgets and/or hooks will fire retroactively).
mw.hook( 'mediawiki.util.addPortletLink'  ).add( function ( item ) {
$(item).addClass( ... ). etc...
} );

I don't think a CSS solution is the right one to go for here.

In BEM one of the fundamental rules is to never nest selectors. A selector .toggle-list__list--drop-down li is thus not BEM. So such a CSS change would need to avoid that confusion by not following BEM.

I see. That wouldn't work without adding a css class to the list items in javascript, thus never work for old gadgets.
As the main menu was using nested selectors, I've followed that pattern. I don't see that BEM would have benefited this specific use-case, it's unfortunate it was tested with this specific part of the DOM.
Removing the selectors of the li and span elements shortened the mustache template generating each list item from ca. 240 to 140 char.

Portlet icons should be different for every menu link, with some kind of default where not specified.

For default, I've added the puzzle icon in patch 5.

Do you want to open the possibility for gadgets to set an icon in the 8th parameter, or a new function? Imho would be nice. The new Vector and Timeless could support it.

This is exactly what I was pushing for initially in T231925#5607253 and https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/545636/ - please see those for background.
Now I'm less sure and think a hook approach might be better.

Yes, have seen that. Custom code for minerva? Hook sounds better.

  1. Minerva subscribes to the hook and massages the DOMElement - adding appropriate classes and icon based on the id of the element e.g. if portlet id is ca-move - it creates an icon `mw-ui-icon-ca-move. This code lives in skins.minerva.scripts (Assuming that will load BEFORE gadgets and/or hooks will fire retroactively).

With the last css it needs to add only the class for the icon to the anchor/button element. I think gadget makers would want to define the icon in the function call, not a callback. Regardless, an initialization hook for skins (minerva) sounds like a good idea.

I don't think a CSS solution is the right one to go for here.

I'm finished with the patch, it has default icon and some cleanup. You can decide whether to go with just js or js+css. A js solution is necessary for customizing the icon, anyway. I find the css+js solution more consistent with other skins and working for old gadgets.

minerva-menu-fix-8-main.png (371×341 px, 11 KB)
minerva-menu-fix-8-user.png (343×270 px, 11 KB)
minerva-menu-fix-8-more.png (310×248 px, 11 KB)

Disclosure: a css solution, which is more consistent with Vector and Timeless would make my task to theme Minerva easier. This was the motivation to make this patch. Consistency in general makes such tasks easier, for ex. theming Vector after Timeless took a very short time, about one hour, as the css selectors are very similar.

In T236711#5835701, @AronManning wrote:

I'm finished with the patch, it has default icon and some cleanup. You can decide whether to go with just js or js+css. A js solution is necessary for customizing the icon, anyway. I find the css+js solution more consistent with other skins and working for old gadgets.

minerva-menu-fix5-main-menu.png (405×378 px, 12 KB)
minerva-menu-fix5-user.png (343×270 px, 11 KB)
minerva-menu-fix5-more.png (307×229 px, 10 KB)

The default icon should have the same color as the other ones. Making it black the default icon stands more than the custom ones.

The default icon should have the same color as the other ones. Making it black the default icon stands more than the custom ones.

Same color after patch 7 as the rest of the minerva icons: #54595d
Also removed the unwanted puzzle icons at bottom of main menu.

minerva-menu-fix-8-main.png (371×341 px, 11 KB)
minerva-menu-fix6-user.png (340×305 px, 12 KB)
minerva-menu-fix6-more.png (316×250 px, 11 KB)
minerva-menu-fix6-more-bulb.png (316×249 px, 11 KB)

Alternative: lightbulb icon. The default icon could be different for each of the 5 menus (main menu has 3).
The unnecessary puzzle icon before "About" and "Disclaimers" has been fixed (removed) in a later patch.

Change 568829 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/MinervaNeue@master] Css cleanup + default icon for "User", "More" drop-down menus and main menu.

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

I've uploaded an alternative along the BEM practices. It's the css and template cleanup part of the previous patch.

In the javascript one class needs to be added to the list items' anchor element: .menu-list-item__button, two for a custom icon: .menu-list-item__button mw-ui-icon-minerva-$iconname.
Benefit: unified css, cleanup in the spirit of UI modernization. Optional patch.

Jdlrobson triaged this task as Medium priority.Feb 3 2020, 2:43 AM
Jdlrobson updated the task description. (Show Details)

Also removed the unwanted puzzle icons at bottom of main menu.

The jigsaw puzzle icons seem suitable to me, but we'd want a designer to chip in.

In T236711#5835782, @AronManning wrote:

Disclosure: a css solution, which is more consistent with Vector and Timeless would make my task to theme Minerva easier. This was the motivation to make this patch. Consistency in general makes such tasks easier, for ex. theming Vector after Timeless took a very short time, about one hour, as the css selectors are very similar.

I've stated my preference - a JS only solution. CSS is very brittle from my experience. As core maintainer of Minerva I am not willing to support inconsistent HTML for different menu items and I have a preference for using BEM in the Minerva repo to minimise specificity wars and UI regressions.

To set expectations, I think this task needs more input from designers and other engineers regarding architecture and we shouldn't merge any gerrit patches until we have agreement on approach here.

As core maintainer of Minerva I am not willing to support inconsistent HTML for different menu items

I'd note that this patch unifies the html (php) and css for the main menu (left side -- #mw-mf-page-left ul li a) and the popup menus (right side -- .toggle-list-item__anchor) into one class: .menu-list-item__button, thereby removing some inconsistency.

and I have a preference for using BEM in the Minerva repo to minimise specificity wars and UI regressions.

This second patch follows the BEM practice, unfortunately losing the ability to provide a default icon for outdated gadgets. I note that I've considered what specificity conflicts are possible in the case of menu items. My theme definitely has to take this into account, but in vanilla mediawiki I don't see a likely chance of .menu-list li ... (used in first, non-BEM patch) being used by an extension with an intent to give it different, conflicting rules than what the skin has. Maybe you know about a case where that happens...

To set expectations, I think this task needs more input from designers and other engineers regarding architecture and we shouldn't merge any gerrit patches until we have agreement on approach here.

Agree. It's marked WIP now. Thanks for the reviews so far.

Change 571111 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Menu items added via addPortletLinks should look like other menu items

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

Change 545636 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] mediawiki.util: addPortletLink fires hook for Minerva UX support

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

It should be possible to customise the implementation of mw.util.addPortletLink function on a skin basis

This sounds like a sensible idea to me. A skins major responsibility is customizing DOM output, so why should skins have to conform to an html structure that might not be suitable for them?
Here's an idea: could we pass a mustache template to mw.util.addPortletLink that would define the HTML output on a per-skin/per-menu basis?

Change 567439 abandoned by Aron Manning:
Style links added by mw.util.addPortletLink() consistent with basic menu items: use css selectors starting at the list root, traversing element tags (consistent with other skins).

Reason:
Working no-js solution

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

@Jdlrobson - I'm having a bit of trouble following. Could you add some more detail on how is applicable to desktop improvements/would it affect the styling of injected links on all skins and how?

Moving to the backlog for the time being as this is not a priority for the team right now. Will get back to this once that changes

Change 571111 abandoned by Jdlrobson:
Menu items added via addPortletLinks should look like other menu items

Reason:
Can be restored and worked on more if the core patch lands.

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

@Jdlrobson I think that should happen in the opposite order. I assure you I'll merge both patches when they are working, I think this is a good idea. But I will not merge additional code into core if there's no guarantee that it is ever going to get used.

Change 571111 restored by Jdlrobson:
Menu items added via addPortletLinks should look like other menu items

Reason:
Per Bartosz

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

Just to note the recent activity - I'm using my 10% time to work on this since the change is small on the assumptions that 1) Bartosz will review so it won't require any input from our team and 2) that the change is wanted and will be well received by community. If any of these assumptions don't hold true please let me know :-)

Change 545636 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.util: addPortletLink fires hook for Minerva UX support

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

Change 571111 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Menu items added via addPortletLinks should look like other menu items

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

matmarex assigned this task to Jdlrobson.

Change 568829 abandoned by Aron Manning:
Css cleanup default icon for "User", "More" drop-down menus and main menu.

Reason:
Some CSS cleanup that could be used to decrease technical debt, but won't be.

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