Page MenuHomePhabricator

[Bug] No page margins for 1000px viewports
Closed, ResolvedPublic

Assigned To
Authored By
Niedzielski
Apr 25 2018, 5:57 PM
Referenced Files
F27086760: Screen Shot 2018-11-06 at 4.50.13 PM.png
Nov 7 2018, 12:53 AM
F27086762: Screen Shot 2018-11-06 at 4.52.05 PM.png
Nov 7 2018, 12:53 AM
F27086765: Screen Shot 2018-11-06 at 4.53.00 PM.png
Nov 7 2018, 12:53 AM
F27086759: Screen Shot 2018-11-06 at 4.51.02 PM.png
Nov 7 2018, 12:53 AM
F25974432: en.m.wikipedia.org_wiki_Barack_Obama_debug=true.png
Sep 17 2018, 6:00 PM
F25974429: en.m.wikipedia.org_wiki_Barack_Obama_debug=true (1).png
Sep 17 2018, 6:00 PM
F25972898: Screen Shot 2018-09-17 at 12.27.10 PM.png
Sep 17 2018, 4:28 PM
F17307143: Screenshot from 2018-04-25 12-47-18.png
Apr 25 2018, 5:57 PM

Description

Steps to Reproduce

  1. Change your viewport to responsive, 1000px wide. You can do this via your web browser's developer tools. (For example in Firefox: Tools → Web Developer → Responsive Design Mode.)
    Screenshot from 2018-04-25 12-47-18.png (810×1 px, 432 KB)
  2. Visit https://en.m.wikipedia.org/wiki/Arkansas_Timberlands

Expected Results

Consistent minimum margins no matter the viewport size.

Actual Results

No margins:

Screenshot from 2018-04-25 12-51-53.png (810×1 px, 466 KB)
en.m.wikipedia.org_wiki_Arkansas_Timberlands.png (1×2 px, 1011 KB)

Environments Observed

  • enwiki

Browser Version

  • Chromium v65.0.3325.181

OS Version

  • Ubuntu v17.10 64b

Device Model

  • Desktop

Device Language

  • English

Developer notes

Per @Jdrewniak 's suggestion we'll define a width of 90% alongside the existing max-width definition.
https://phabricator.wikimedia.org/T193061#4590922

QA

Test this one on https://reading-web-staging.wmflabs.org

Event Timeline

I chatted to @alexhollender on Monday about supporting different official viewports. Right now we only officially support in Minerva are 320p and, 720px The more we support the more complicated/complex it is to keep that support level high and I'm not sure we can scale to this kind of work without switching to a true fluid layout.

When discusssing this we felt it might be helpful to collect some data around what browser resolutions people are viewing Minerva in before doing any work in this area.

I've started talking with @Tbayer about what it would take to collect screen size data for readers. Until we have more specific data I thought it might make sense to look at general statistics. These two sites report on "screen resolution" — I'm not sure if that's quite the same thing as the size of your browser window?

https://www.rapidtables.com/web/dev/screen-resolution-statistics.html
http://gs.statcounter.com/screen-resolution-stats

If we think it's a decent proxy, it seems like we should probably consider introducing a third breakpoint around 1200px. There could even be an argument that we stop caring about 720px.

@alexhollender tbh those general stats may be enough. The statistics only become useful when thinking about who is using the mobile site, how they make use of their screen (maybe on a 1000px they are not full screening their windows and should we be caring more about 720px upwards. If so though we've got bigger problems - such as the editor overlay appearing full screen.

I should note this particular bug is about 1000px. The design looks like nice on 1024px which features in the table with 3.9% usage.

@Jdlrobson right, good point. Given that I don't think this is high priority.

I guess another way we could think about this is having a 2-3 main breakpoints, but still doing things that help the display of the page in between those breakpoints. E.g. in this case it seems like if we set a percentage-based width for the main container (90%), and introduced a pixel based max-width we'd avoid this problem.

Vvjjkkii renamed this task from [Bug] No page margins for 1000px viewports to i8daaaaaaa.Jul 1 2018, 1:13 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed a subscriber: Aklapper.
1339861mzb renamed this task from i8daaaaaaa to [Bug] No page margins for 1000px viewports.Jul 1 2018, 3:37 PM
1339861mzb updated the task description. (Show Details)
1339861mzb updated the task description. (Show Details)

@Jdlrobson @Niedzielski what if we changed the max-width here to 90% instead of 993.3px? As far as I can tell that would ensure there is always some margin.

Screen Shot 2018-09-17 at 12.27.10 PM.png (154×342 px, 28 KB)

@alexhollender, I don't think we want to do that since 10% gives too wide a page on large screens. Why not define the margins wanted instead per breakpoint?

en.m.wikipedia.org_wiki_Barack_Obama_debug=true (1).png (1×3 px, 1 MB)

en.m.wikipedia.org_wiki_Barack_Obama_debug=true.png (2×3 px, 1 MB)

Why not define the margins wanted instead per breakpoint?

The content needs to be centered so a margin: auto will be needed somewhere, if not here on a parent container.

The fundamental issue here is that the breakpoint is 1000px, and the content area does not have a width of 993.3px available. The easiest solutions here would be dropping that value of that max-width or increasing the breakpoint at which it comes into play (a little trickier as impacts other things.

we could also define a width + a max-width, let's say

max-width: 993.3px;
width: 90%;
margin-left: auto;
margin-right: auto;

That way the content will be never be bigger than 993.3px, and on smaller screens it'll be 90% width.

Nice @Jdrewniak — just tried that out on a large screen and it seems like it does exactly what we want.

Can I move this out of design land?

ovasileva triaged this task as Medium priority.Sep 18 2018, 10:24 PM
ovasileva edited projects, added Web-Team-Backlog (Design); removed Web-Team-Backlog.
alexhollender_WMF added a subscriber: ovasileva.

@ovasileva for clarification this is ready to be worked on, no more design needed

Change 469836 had a related patch set uploaded (by Goomel; owner: Goomel):
[mediawiki/skins/MinervaNeue@master] Bug: T193061

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

Change 469836 had a related patch set uploaded (by Jdlrobson; owner: Goomel):
[mediawiki/skins/MinervaNeue@master] Ensure there are page margins for 1000px viewports

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

Change 469836 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Ensure there are page margins for 1000px viewports

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

Jdlrobson added a subscriber: nray.

@nray points out there is a problem with the implementation - it's taking up the full screen on desktop.

Change 470886 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Article width should not be full screen

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

Change 470887 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@wmf/1.33.0-wmf.2] Article width should not be full screen

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

@alexhollender am requesting an urgent sign off by 2pm PST (2.5hrs from now) on http://reading-web-staging.wmflabs.org so I can SWAT this at 4pm PST.

LGTM. Checked at various browser widths.

Change 470886 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Article width should not be full screen

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

Change 470887 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@wmf/1.33.0-wmf.2] Article width should not be full screen

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

Mentioned in SAL (#wikimedia-operations) [2018-10-31T23:34:26Z] <tgr@deploy1001> Synchronized php-1.33.0-wmf.2/skins/MinervaNeue/resources/skins.minerva.tablet.styles/common.less: SWAT: [[gerrit:470887|MinervaNeue: Article width should not be full screen (T193061)]] (duration: 00m 53s)

Mentioned in SAL (#wikimedia-operations) [2018-10-31T23:36:30Z] <tgr@deploy1001> Synchronized php-1.33.0-wmf.2/skins/MinervaNeue/skin.json: SWAT: [[gerrit:470887|MinervaNeue: Article width should not be full screen (T193061)]] (invalidate RL cache) (duration: 00m 53s)

Jdlrobson added a project: Product-QA.

This can possibly skip QA because at least @nray , myself and @alexhollender have looked at it, but it doesn't hurt for one more person to look at it now that we're live in production.

looks good, resolving