Page MenuHomePhabricator

Top alignment for Page Issues Modal
Closed, ResolvedPublic2 Story Points

Description

T187916: Improve Page Issue modal
After we finished work on modal, we have 1 minor follow up tweak around this work

The top alignment for text and the icon

Note: modal name should be "Page issues" (as is currently)

Developer notes

Can be seen here: https://en.m.wikipedia.org/wiki/Offset_printing

While tempting to add a top: -2px we should investigate the route cause - the HTML elements themselves are aligned but the paragraph o text gets top padding from somewhere. We should not incur tech debt as part of this change. We may need to look at the implementation of mw-ui-icon

Testing Criteria

  1. visit http://reading-web-staging.wmflabs.org/wiki/Infrastructure?useformat=mobile#/issues
  2. ensure that the top of the icon (orange circle) is aligned with the top of the text ("This article...")
  3. check on multiple browsers (mobile & desktop)
  4. repeat steps 1–3 with http://reading-web-staging.wmflabs.org/wiki/Rowing_(sport)?useformat=mobile#/issues

Related Objects

Event Timeline

Nirzar created this task.Mar 22 2018, 10:14 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 22 2018, 10:14 PM

@alexhollender to followup on these

ovasileva triaged this task as Normal priority.Mar 23 2018, 11:47 AM

I'm seeing "Page issues" as the modal title


Jdlrobson renamed this task from follow up fixes for Page Issues Modal to Top alignment for Page Issues Modal.Mar 28 2018, 1:41 PM
Jdlrobson updated the task description. (Show Details)
ovasileva updated the task description. (Show Details)Mar 28 2018, 1:59 PM
ovasileva set the point value for this task to 2.Mar 28 2018, 4:14 PM
ovasileva moved this task from Upcoming to 2017-18 Q3 on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)Mar 28 2018, 4:14 PM

You can use the ::first-line selector and specify a line-height of 1 to achieve this. Not sure if that method is allowed.

Jdlrobson moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

The empty p is probably the problem here...

Change 422464 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Issue line height makes icon and text vertically aligned

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

Jdlrobson removed Jdlrobson as the assignee of this task.Mar 28 2018, 6:52 PM

Change 422464 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Issue line height makes icon and text vertically aligned

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

This achieves the top alignment but appears to have the unintended consequence of the line-height being 1 for all the text in the modal

(related to your comment above: when I delete that empty <p> tag from the HTML the problem persists)

Change 423066 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Issue line height makes icon and text vertically aligned

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

Jdlrobson added a subscriber: pmiazga.EditedMar 29 2018, 11:26 PM

I've gone with the first-line technique.I cannot find a better solution and the time investment I'm making fighting for one is not worth it.

It's on staging for design review (but also needs code review!).

@pmiazga @alexhollender we'll need to merge this before Tuesday AM UTC to make sure it doesn't go out in current form so this is time sensitive.

Change 423066 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Issue line height makes icon and text vertically aligned

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

alexhollender updated the task description. (Show Details)

Staging has been updated with latest master branch.

Looks pretty good on staging across a selection of browsers, except for Firefox. It looks a little high to me, but not terribly so. @alexhollender, can you take a look at firefox and if it's good to you, we can move to PM signoff.

android/chrome - android/android - android/opera

Android/firefox

ios/safari

Talked with Nirzar. It's okay to ignore Firefox here. Thanks for catching that @ABorbaWMF.

ovasileva closed this task as Resolved.Apr 4 2018, 10:29 AM

Looks good then - thanks all!