Page MenuHomePhabricator

Use ResourceLoaderImageModule in the Popups extension for settings icon (not mwe-popups-settings)
Closed, ResolvedPublic1 Estimated Story Points

Description

The result of the spike at T131314#2250027 (Review Icon support to see if it will benefit from ResourceLoaderImageModule):

The extension uses only two icons - cog and horn.

The benefits over the current implementation are:

No need to keep the generated PNGs around, RL will generate them on the fly;
Standard mw-ui-icon, rather than the current custom styled icon.
Future potential benefits are:

Specify different images for RTL and LTR languages if we want to do so;
Multi-colored images based on the single SVG.

Acceptance criteria:

  • Switch the cog icon implementations to use ResourceLoaderImageModule and remove the css rule for .mwe-popups-settings
  • Remove the unnecessary png.

Test Plan

  1. Enable Page Previews
  2. Visit any page on the Beta Cluster (https://en.wikipedia.beta.wmflabs.org/wiki/Special:Random)
  3. Dwell on a link for long enough that a preview appears
  4. Observe that the settings cog is visible
  5. Tap the settings cog and get taken to the preferences page (or the settings modal if you're logged out)

Event Timeline

Jdlrobson renamed this task from Use ResourceLoaderImageModule in the Popups extension to Use ResourceLoaderImageModule in the Popups extension for settings icon (not mwe-popups-settings).Feb 15 2017, 10:14 PM
Jdlrobson raised the priority of this task from Low to Medium.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: good first task.
Jdlrobson set the point value for this task to 0.5.
Jdlrobson subscribed.

We are using ResourceLoaderImage module for the close icon but not for the settings cog. This should be easy to fix and I recommend we do that soon so we don't forget.

Jdlrobson changed the point value for this task from 0.5 to 1.Mar 8 2017, 5:36 PM
Jdlrobson moved this task from Needs Analysis to To Do on the Reading-Web-Sprint-94 board.

Change 349344 had a related patch set uploaded (by Jdlrobson):
[mediawiki/extensions/Popups@master] Generate cog icon via ResourceLoaderImage module

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

During testing, I found that rEPOP9e48696cb8a0: Generate cog icon via ResourceLoaderImage module breaks the generic preview across all browsers, e.g. in IE9 on Windows 7 I see:

Screen Shot 2017-04-21 at 10.29.32.png (206×338 px, 16 KB)

The settings cog, however, works as expected.

So far this hasn't proved as easy as I first hoped. Seems there's a lot of CSS depending on the size of that icon being a certain way. I'll try to unravel what's going on and make sure the icon still gets placed in the correct place.

New patch is up and I'm hopeful it should remove the bugs associated with the last. If there's any issues with how it's arranged let's go through them Monday and iron them out in a pair programming session.

Quoting my review:

This LGTM. I've tested the change by triggering generic and normal previews in the following browsers:

  • macOS Sierra (10.12.3) – Chrome (58.0.3029.81)
  • macOS Sierra (10.12.3) – Safari (10.0.3)
  • macOS Sierra (10.12.3) – Firefox (53.0)
  • Windows 7 – IE9
  • Windows 10 – IE11
  • Windows 10 – Edge (latest)

/cc @ABorbaWMF

Change 349344 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Generate cog icon via ResourceLoaderImage module

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

I am still seeing the problem on beta when hovering over link with a missing preview. I tried clearing the cache. Will try again tomorrow.

image.png (768×1 px, 308 KB)

@ABorbaWMF: That's per the design. "Generic previews", previews that appear after dwelling on a link that can't be previewed, don't show the settings cog.

Tested on https://en.wikipedia.beta.wmflabs.org

Tried a number of system and browser combinations. Appears to be fixed.

phuedx added a subscriber: ovasileva.

I'll be the signer offerer for PP tasks (that I haven't submitted changes for) while @ovasileva is OOO.

Per T133956#3211694, this is 👍