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

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

Details

	Subject	Repo	Branch	Lines +/-
	Menu icons should be using ResourceLoaderImage module	mediawiki/extensions/MobileFrontend	master	+64 -51
	Correctly render the watchlist icon	mediawiki/extensions/MobileFrontend	master	+2 -1

Customize query in gerrit

Related Objects
Search...

		Status	Subtype	Assigned	Task
		Resolved		Jdlrobson	T131931 Regression: Oojs ui icons replace mw-ui-icons when Echo used
		Resolved		Jdlrobson	T133156 Rename watchlist icon in main menu to not collide with OOJS

Event Timeline

• Jhernandez created this task.Apr 20 2016, 3:18 PM

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

dr0ptp4kt moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 20 2016, 3:57 PM

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

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

gerritbot added a project: Patch-For-Review.Apr 21 2016, 9:02 PM

dr0ptp4kt moved this task from To Do to Code Review on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 25 2016, 4:20 PM

Jdlrobson moved this task from Code Review to -1 (Needs More Work) on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 25 2016, 11:55 PM

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)

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

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

ReleaseTaggerBot added a project: MW-1.27-release (WMF-deploy-2016-04-26_(1.27.0-wmf.22)).Apr 26 2016, 1:00 AM

• bmansurov moved this task from -1 (Needs More Work) to To Do on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 26 2016, 1:20 PM

Jdlrobson lowered the priority of this task from High to Medium.Apr 26 2016, 5:36 PM

Jdlrobson removed a project: Patch-For-Review.

MBinder_WMF added a project: Web-Team-Backlog.Apr 27 2016, 5:09 PM

MBinder_WMF moved this task from Incoming to 2015-16 Q4 on the Web-Team-Backlog board.Apr 27 2016, 5:13 PM

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 27 2016, 10:29 PM

Jdlrobson moved this task from Doing to To Do on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.

Jdlrobson claimed this task.Apr 27 2016, 11:20 PM

Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.

Jdlrobson moved this task from Doing to Code Review on the Reading-Web-Sprint-71-Matisse-Monet-Kandinsky-and-the-Departing-Painters board.Apr 28 2016, 12:30 AM

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

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

gerritbot added a project: Patch-For-Review.Apr 28 2016, 12:31 AM

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.

In T133156#2248362, @phuedx wrote:

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.