Page MenuHomePhabricator

Use ResourceLoaderImageModule in the Popups extension for settings icon (not mwe-popups-settings)
Closed, ResolvedPublic1 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)

Details

Related Gerrit Patches:
mediawiki/extensions/Popups : masterGenerate cog icon via ResourceLoaderImage module

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 28 2016, 10:11 PM
bmansurov updated the task description. (Show Details)Apr 28 2016, 10:12 PM
Jdlrobson triaged this task as Low priority.May 10 2016, 5:16 PM
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 added a subscriber: Jdlrobson.

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.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptFeb 15 2017, 10:14 PM
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.
Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-96 board.Apr 20 2017, 9:54 PM

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

phuedx added a subscriber: phuedx.Apr 21 2017, 9:36 AM

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:

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

phuedx updated the task description. (Show Details)
phuedx reassigned this task from Jdlrobson to ABorbaWMF.Apr 24 2017, 2:40 PM

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.

@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.

phuedx updated the task description. (Show Details)Apr 25 2017, 4:04 PM

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

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

phuedx claimed this task.Apr 26 2017, 9:40 AM
phuedx updated the task description. (Show Details)Apr 26 2017, 9:48 AM
phuedx closed this task as Resolved.Apr 26 2017, 9:56 AM
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 👍