Page preview margins should be consistent
Closed, ResolvedPublic3 Story Points

Description

Problem

For vertical page previews the margin between the bottom of the image and the first line of text is inconsistent, based on whether the page preview opens above or below the linked text. Here is a screenshot of me hovering over the same link, once where it's towards the top of the screen and once where it's towards the bottom:


Design notes

The desired amount of padding is 25px

QA

Visit http://reading-web-staging.wmflabs.org/wiki/Glacier?useformat=desktop
Trigger page previews on "ice sheets" for both orientations.

Developer notes

I tried to debug this a little bit but the inspector was acting a little funky with the $($0).trigger('mouseenter'); trick. It kept opening the page preview above the link even if the link was super close to the top of the screen.

This seems like an oversight.
The rule

.mwe-popups.mwe-popups-image-pointer .mwe-popups-extract { padding-top: 32px; }

can easily be changed to padding-top: 25px to give consistency.

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 2 2018, 9:25 PM
alexhollender updated the task description. (Show Details)Jul 2 2018, 9:29 PM
ovasileva triaged this task as Normal priority.Jul 3 2018, 1:58 PM
ovasileva moved this task from Backlog to Needs Analysis on the Page-Previews board.
ovasileva moved this task from To Triage to Needs Analysis on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)Jul 3 2018, 7:51 PM
Jdlrobson moved this task from Needs Analysis to Triaged but Future on the Readers-Web-Backlog board.
pmiazga set the point value for this task to 3.Jul 4 2018, 4:29 PM
pmiazga added a subscriber: pmiazga.Jul 4 2018, 4:32 PM

We estimated it 3 points because we encounter many small issues related to how Page Previews looks like (margins, clickable areas, wrong positioning of the image). I would encourage to check the Page Previews rendering code and try to fix in the way we do not get more small issues like this one.

Jdlrobson claimed this task.

Change 445235 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Tweak page previews margin for consistency

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

@alexhollender:
After above change this is how they are looking:


Jdlrobson removed Jdlrobson as the assignee of this task.Jul 11 2018, 9:15 PM
Jdlrobson added a subscriber: Jdlrobson.

@Jdlrobson is there extra space getting added somewhere? I'm measuring 30px instead of 25px.

Change 445235 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tweak page previews margin for consistency

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

Jdlrobson updated the task description. (Show Details)Jul 12 2018, 9:30 PM

@Jdlrobson is there extra space getting added somewhere? I'm measuring 30px instead of 25px.

I'm measuring 25px. Want to check out http://reading-web-staging.wmflabs.org/wiki/Glacier?useformat=desktop ?
All popups are coded to have a 16px margin for both landscape and horizontal versions
e.g.

However there is also a negative margin on the top of the popup of -9px (.mwe-popups .mwe-popups-container)

When the image is on top, this negative margin increases the space between the image and extract by 9px to 25px;
This is not documented anywhere and I'm not sure if this is intentional.
So should it be 25px as it is now, or 16?

So should it be 25px as it is now, or 16?

@alexhollender, is this 25px then? If so, I'm going to move this to QA!

@Niedzielski the current patch is good to go

Niedzielski added subscribers: ABorbaWMF, Ryasmeen.

@Ryasmeen / @ABorbaWMF, this is ready for QA:

  1. Visit https://readers-web-master.wmflabs.org/w/index.php?title=Glacier&mobileaction=toggle_view_desktop.
  2. Trigger page previews on "ice sheets" for both orientations.

Looks good on master
above


below

ovasileva closed this task as Resolved.Jul 19 2018, 8:41 AM
ovasileva added a subscriber: ovasileva.

all done!

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptJul 19 2018, 8:42 AM