Page MenuHomePhabricator

Page previews cog should use OOUI settings icon
Closed, ResolvedPublic

Description

Prior art: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/503343/

Acceptance criteria

  • Asset is updated
  • Popups is using a ResourceLoaderIcon pack with minimal local overrides.

Event Timeline

Change 636357 had a related patch set uploaded (by Jdlrobson; owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Update settings (cog) icon with optimized code from OOUI 0.32.0

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

Change 636710 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Settings cog should come from icon pack

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

Change 636357 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Update settings (cog) icon with optimized code from OOUI 0.32.0

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

I might be wrong about the following, but I wonder if this change might increase the page load? Note this code is loaded for every single page view (at least in the article namespace). The patch replaces a tiny .svg file with a larger set of OOUI icons. This is not necessarily bad. The total page load will be smaller in situations where this icon set is loaded anyway, but larger if not. Do we know which situation is more common? @Krinkle, does the performance team want to have a look here?

I manually tested the patch. As far as I can see it looks fine. Below is a comparison:

  • Before the patch.
  • After the patch, as of patch set 3.
  • Same, but when I remove this line here.

Screenshot from 2020-10-28 11-13-30__.png (240×643 px, 34 KB)

Note the gray rectangle is when you hover the icon with the mouse. I intentionally included this in the screenshots.

Personally, I find both #2 and #3 acceptable, with a tendency towards #3, because it is closer to the original design (and respects the mw-ui-icon-small style). @ECohen_WMDE, maybe you want to have a look?

I might be wrong about the following, but I wonder if this change might increase the page load?

The newly created icon pack should only contain one icon so should be the same size. There's a small bump in the startup module but that will be reclaimed when the other icons have been migrated there.

Change 636710 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Settings cog should come from icon pack

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

@thiemowmde Thanks for tagging me, sorry this fell through the cracks. I agree both 2 & 3 are acceptable and I'm comfortable with RefPreviews using whichever the page previews team chose to implement.

Likely needs design review/QA to ensure the settings cog on beta cluster is acceptable.

I'm not seeing much of a difference between beta cluster and production. The only thing I notice is that on beta the icon is properly centered within the gray square, whereas on production it's a bit off-center vertically (too high up). @ECohen_WMDE thoughts?

image.png (980×888 px, 382 KB)

@alexhollender Agree, not much seems changed but the fact that it's centered seems like a nice improvement. Also seems like the icon is a bit larger, but that doesn't look like a problem to me. I think it'll be fine to eventually use the same, updated version on RefPreviews when we add the cog there.

ovasileva claimed this task.
ovasileva added a subscriber: ovasileva.

All done