Page MenuHomePhabricator

Rename watchlist icon in main menu to not collide with OOJS
Closed, ResolvedPublic1 Story Points

Description

  • Merge original patch to get a fix riding the train

To avoid future medium-term issues to main menu:

  • Prefix all MobileFrontend menu icons with mw-ui-icon-mf- and drop the .menu parent selector
  • Test on cached HTML and verify the fix works

Event Timeline

MBinder_WMF set the point value for this task to 1.Apr 20 2016, 3:20 PM

Change 284808 had a related patch set uploaded (by Bmansurov):
Correctly render the watchlist icon

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

Note that I merged the original patch so that a fix rides the train but I think we should clean this up in some follow ups based on this conversation so I've moved to -1 column.

This one is a little tricky because although the menu is being generated with JS, the data is coming from the backend: https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/search?utf8=%E2%9C%93&q=wgMinervaMenuData that's why I thought the menu items would be cached. Do you think that's not the case?
JDLR: We should check this on a cached HTML. Since the template is generated in JS via ResourceLoader (5min cache) I think we are okay.
I would prefer we stay compatible with ResourceLoaderImage module to avoid complicating the future fix for that bug.
Are you suggesting that we fix the above bug you mentioned? Renaming the icon won't satisfy this requirement either.
JDLR: In a follow up since we are here in this piece of code, this might be a good time.
The existing fix also assumes we may never want to use the icon elsewhere outside of the menu.
This is true, and we have a pretty good chance of not using the watchlist icon outside menu until T131931 is fixed. We haven't used that icon in another place for so long, and I don't expect us to make drastic changes to the UI in the coming months.
JDLR: Let's follow your suggestion in the parent bug and simply prefix all these menu icons with mf- to keep us safe in future given the main menu is so prominent (we may later want to do this for page actions too but let's hold off on that for the time being)

Jdlrobson updated the task description. (Show Details)Apr 25 2016, 11:58 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

Change 284808 merged by jenkins-bot:
Correctly render the watchlist icon

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

Jdlrobson lowered the priority of this task from High to Normal.Apr 26 2016, 5:36 PM
Jdlrobson removed a project: Patch-For-Review.

Change 285889 had a related patch set uploaded (by Jdlrobson):
Menu icons should be using ResourceLoaderImage module

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

phuedx added a subscriber: phuedx.Apr 28 2016, 12:55 PM

From my review of PS1 of 285889: Menu icons should be using ResourceLoaderImage module:

The change LGTM but I have to ask: why "mw-ui-icon-mf" not "mf-icon"? I feel that, at least, the ui prefix is redundant and mf is specific enough to warrant dropping the mw prefix too.

From my review of PS1 of 285889: Menu icons should be using ResourceLoaderImage module:

The change LGTM but I have to ask: why "mw-ui-icon-mf" not "mf-icon"? I feel that, at least, the ui prefix is redundant and mf is specific enough to warrant dropping the mw prefix too.

Exactly, if we want OOJS-UI to not use mw-ui-icon, then we should not use it ourselves either.

To be clear I strongly oppose us creating our own version of mw-ui-icon called mf-icon. This is backwards and given I expect the parent bug to be fixed asap this should be unnecessary (when the parent bug is fixed we should not have this issue). We should continue to use mw-ui-icon. OOJS-UI does not use mw-ui-icon it just has a side effect of creating mw-ui-icon compatible glyph classes.

Why mw-ui-icon-mf and not mf-icon?
This is how mw-ui-icon's work.
To be clear the prefix is only added on the glyph name. Take a look at - Icon::getGlyphClassName. It is not added for all the other classes - this would cause huge issues with cached pages and would require us reimplementing mw-ui-icon in MobileFrontend and it is not worth the work.

Change 285889 merged by jenkins-bot:
Menu icons should be using ResourceLoaderImage module

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