Regression: Icon hacks in Popups need to be revised
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Jdlrobson
	Jul 20 2021, 11:28 PM

Description

Vector's icon styles have been updated in T191021. Unfortunately, but expectedly this has led to some icon issues across the page previews project.
This must be fixed before 1.37.0-wmf.16.

Impact:
Several of the padding on icons is incorrect / icon sizes incorrect
Examples:

Screen Shot 2021-07-20 at 4.49.04 PM.png (554×728 px, 130 KB)

Screen Shot 2021-07-20 at 4.48.31 PM.png (600×810 px, 173 KB)

As part of this change it likely makes sense to create a storybook for reference tooltips for testing before and after.

Fixing this involves:

removing any hacks which try to correct the previously incorrect icon dimensions
revising positioning of icons (settings cog, reference tooltips headings
removing the default icon hover effect

Details

	Subject	Repo	Branch	Lines +/-
	Remove the page preview icon hacks	mediawiki/extensions/Popups	master	+18 -49

Customize query in gerrit

Related Objects

Mentioned In: T287520: Regression: Some page previews show extra white space beneath the image
Mentioned Here: T287520: Regression: Some page previews show extra white space beneath the image
T191021: User links: Standardize `.mw-ui-icon` to overhauled icon canvas size 20x20

Event Timeline

Jdlrobson created this task.Jul 20 2021, 11:28 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 20 2021, 11:28 PM

Jdlrobson claimed this task.Jul 20 2021, 11:30 PM

Jdlrobson triaged this task as High priority.

Jdlrobson updated the task description. (Show Details)

Jdlrobson removed Jdlrobson as the assignee of this task.Jul 20 2021, 11:40 PM

Jdlrobson moved this task from Doing to Code Review on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.

FYI @Lena_WMDE we may need some timely input from WMDE on this change to make sure we restore this correctly.

Jdlrobson added a parent task: T281158: 1.37.0-wmf.17 deployment blockers.Jul 21 2021, 1:21 AM

Thanks @Jdlrobson! I'm adding it to our sprint to provide feedback

I'm not sure if it belongs into this ticket, but I looked at the current core master that includes[1] in combination with the current Vector (legacy skin) master that includes [2] in combination with the patch for Page-Previews [3]

Here are some before and after screenshots from different reference preview types:

RefPreviews NewLayout - Note - Before.png (360×580 px, 49 KB)

RefPreviews NewLayout - Note - After.png (360×580 px, 46 KB)

RefPreviews NewLayout - Reference - Before.png (360×680 px, 47 KB)

RefPreviews NewLayout - Reference - After.png (360×680 px, 46 KB)

RefPreviews NewLayout - WebReference - After.png (360×580 px, 43 KB)

RefPreviews NewLayout - WebReference - Before.png (360×580 px, 42 KB)

RefPreviews NewLayout - Figure - Before.png (360×580 px, 50 KB)

RefPreviews NewLayout - Figure - After.png (360×580 px, 48 KB)

[1] https://gerrit.wikimedia.org/r/c/mediawiki/core/+/704987
[2] https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/702456
[3] https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/705510/7/

@Jdlrobson Just to be sure about that: WMDE is supposed to give review input not necessarily fixing the issues ourselves by creating own patches, right?

Andrew-WMDE moved this task from Sprint Backlog to Watching (Stalled / Epic) on the WMDE-TechWish-Sprint-2021-07-21 board.Jul 21 2021, 11:59 AM

@WMDE-Fisch review input only should be enough. My goal is to make sure you are happy with what gets shipped on next week's train.

Jdlrobson assigned this task to • nray.Jul 21 2021, 4:42 PM

Change 705510 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] Remove the page preview icon hacks

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

gerritbot added a project: Patch-For-Review.Jul 21 2021, 5:00 PM

Change 705510 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Remove the page preview icon hacks

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

@WMDE-Fisch
we think we've covered all the issues here with reference tooltips but could you review the beta cluster. Please also check out the storybook patch .

@alexhollender
Could you review the page previews product on beta cluster and https://doc.wikimedia.org/Popups/master/js/ui/
for any icon issues

Please move to QA, needs more work or blocked on others depending on whether WMDE have had a chance to review.

When reporting any issues please make clear if they are considered blockers for next week train. You should also feel free to submit follow up patches if it's easier to write patches than to explain on phabricator.

ReleaseTaggerBot added a project: MW-1.37-notes (1.37.0-wmf.16; 2021-07-26).Jul 22 2021, 12:00 AM

Maintenance_bot removed a project: Patch-For-Review.Jul 22 2021, 12:10 AM

Thanks @Jdlrobson, looks good to me. The slight differences mostly feel like improvements. For full transparency I'll add another set of before/after screenshots in here. Also just FYI pinging @ECohen_WMDE @Sarai-WMDE and @Lena_WMDE

before

after

before

after

before

after

I'll also take a look at the storybook patch.

@Jdlrobson I'm seeing two things:

the hover area of the cog is meant to be 30x30px, currently it's 34x34px

Even though the padding looks equal in the inspector, it seems like the cog is 2px too close to the vertical edge of the preview:

Screen Shot 2021-07-22 at 3.36.42 PM.png (629×1 px, 39 KB)

also, yesterday we discussed Reference previews — should I look at those as well? Not seeing them in the storybook

• alexhollender_WMF reassigned this task from • alexhollender_WMF to Jdlrobson.Jul 22 2021, 7:37 PM

• alexhollender_WMF moved this task from Design Review to Needs More Work on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.

Change 706713 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/UniversalLanguageSelector@master] Restrict compact language button styles to legacy Vector

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

gerritbot added a project: Patch-For-Review.Jul 22 2021, 7:43 PM

@alexhollender I think the issue with the settings cog is it's a small icon

In T287058#7231186, @alexhollender wrote:

@Jdlrobson I'm seeing two things:

the hover area of the cog is meant to be 30x30px, currently it's 34x34px

This is actually a problem with the mw-ui-icon specification.

Assuming this is not a blocker for next weeks deployment, is it okay to handle this in a separate ticket? There may be other concerns given its usage in Minerva for the section toggle which is also 34x34 and presumably should be 30x30 ?:

Screen Shot 2021-07-22 at 1.05.29 PM.png (214×592 px, 22 KB)

Even though the padding looks equal in the inspector, it seems like the cog is 2px too close to the vertical edge of the preview:

It looks like the footer width is slightly too big in certain cases. I'm seeing a footer of size 322px in a popup container of 320px which would explain those 2px misalignment. This is unrelated to the icons change, likely an existing issue and worthy of a new ticket.

also, yesterday we discussed Reference previews — should I look at those as well? Not seeing them in the storybook

The storybook patch hasn't been merged yet. WMDE are reviewing that one. In the mean time you'll need to use the beta cluster unfortunately.

Jdlrobson moved this task from Needs More Work to Design Review on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Jul 22 2021, 10:50 PM

@Jdlrobson sounds good. no deployment blockers. I will follow up next week with the respective tasks. not sure where to move this next.

Moving to sign off and leaving you as an assignee until the tasks are created and you are happy with them.

Jdlrobson mentioned this in T287520: Regression: Some page previews show extra white space beneath the image.Jul 27 2021, 10:53 PM

This comment was removed by Jdlrobson.

LGoto edited projects, added Web-Team-Backlog (Kanbanana-FY-2021-22); removed Web-Team-Backlog (Kanbanana-FY-2020-21).Aug 2 2021, 9:30 PM

LGoto moved this task from Needs Analysis to Ready for Signoff on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.

Agusbou2015 subscribed.Aug 4 2021, 8:42 PM

This must be fixed before 1.37.0-wmf.16.

I notice that this was a blocker to wmf.17, but we deployed wmf.17 everywhere. What happened? Should this block wmf.18?

@thcipriani not a blocker. This is done, just waiting the kind of sign off only a designer can provide.

In T287058#7267775, @Jdlrobson wrote:

@thcipriani not a blocker. This is done, just waiting the kind of sign off only a designer can provide.

👍 thanks @Jdlrobson

looks great

gear in page previews.gif (906×1 px, 416 KB)

	F34560378: Screen Shot 2021-07-22 at 1.07.51 PM.png
	Jul 22 2021, 8:11 PM

	F34560375: Screen Shot 2021-07-22 at 1.05.29 PM.png
	Jul 22 2021, 8:11 PM

	F34560359: Screen Shot 2021-07-22 at 3.36.42 PM.png
	Jul 22 2021, 7:37 PM

	F34559644: 22.png
	Jul 22 2021, 8:16 AM

	F34559646: 31.png
	Jul 22 2021, 8:16 AM

	F34559638: 11.png
	Jul 22 2021, 8:16 AM

Regression: Icon hacks in Popups need to be revisedClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

Regression: Icon hacks in Popups need to be revised
Closed, ResolvedPublic
Actions