Page MenuHomePhabricator

Regression: Icon hacks in Popups need to be revised
Open, HighPublic

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

Event Timeline

Jdlrobson triaged this task as High priority.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Lena_WMDE.

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

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 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?

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

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

Change 705510 merged by jenkins-bot:

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

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

Jdlrobson added subscribers: alexhollender, nray.

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

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

11.png (500×1 px, 67 KB)

after
12.png (500×1 px, 67 KB)

before

21.png (500×1 px, 73 KB)

after
22.png (500×1 px, 73 KB)

before

31.png (500×1 px, 66 KB)

after
32.png (500×1 px, 66 KB)

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

@Jdlrobson I'm seeing two things:

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

image.png (255×329 px, 20 KB)

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

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

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

@Jdlrobson I'm seeing two things:

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

image.png (255×329 px, 20 KB)

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.

Screen Shot 2021-07-22 at 1.07.51 PM.png (192×906 px, 38 KB)

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

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.

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

👍 thanks @Jdlrobson