Page MenuHomePhabricator

Make multiple images template work on mobile again
Closed, ResolvedPublic

Description

In T38030: Inline styles: Mobile Frontend doesn't display multiple horizontal images well we added a workaround for a breakage with the Multiple image template (Change-ID I6448b077a10e84a4742350fe35286d483f5c4eff)

.thumb .thumbinner > div {
	float: none !important;
	width: auto !important;
	clear: both !important;
}

This workaround has since made it into resources/skins.minerva.content.styles/thumbnails.less even though it is theoretically specific to the English Wikipedia (though possibly it is used on more wiki's).

Since then we have made many more improvements to how images and thumbnails are layed out and the template also has had a change. It seems that we no longer need this style rule and can safely disable it, with the template automatically wrapping to a new line when needed now.

Attached a screenshot, showing the wrapping behavior, with this style rule disabled.


Tested with the same article as the original report.

When disabling this, I have not observed significant breakage with galleries, video players, wide/panorama images and other elements that I could think of. Only tested with Safari however.

Even when this change is not made, I suggest annotating that line with the bug ticket number that introduced it, for future reference.

Event Timeline

TheDJ created this task.Oct 18 2016, 12:21 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 18 2016, 12:21 PM

Change 316551 had a related patch set uploaded (by TheDJ):
Remove thumbnail style rule that is no longer required

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

Hi @TheDJ sorry I missed the irc conversation.
These rules are there for mobile browsers.

It was a deliberate design choice to center such thumbnails in the page rather than float them:


Note that some images are < 120px in which case the floating becomes more relevant as the text and image get jammed together.

This rule shouldn't be removed but it could be wrapped in a media query to scope to under 720px-ish. Generally we take a mobile first approach to our css to support the really old mobile devices. TemplateStyles would allow us to use classes and to scope those css rules with media queries rather than using an inline style.

TheDJ added a comment.Oct 18 2016, 7:51 PM

you mean the image clashing with the caption of the a previous element, if said caption is much longer than the caption of a subsequent caption ?

That's a rather rare though isn't and still quite readable.... I would argue much rarer than people complaining about template:multiple_image not doing what it is supposed to be doing...

TheDJ added a comment.EditedOct 18 2016, 8:00 PM

or do you mean as in actually centering inside the viewport? Like it already does with small images even these rules are disabled:

The centering is only not present if the images are too large to fit on a single line together. And WITH those rules, the exact example doesn't do that either..

To me, that seems like too little benefit.

Do also note, that we are talking about .thumb .thumbinner > div (there is another style rule with the same styling statements as well).

Jdlrobson assigned this task to Nirzar.Oct 18 2016, 9:16 PM
Jdlrobson added a project: Design.

can someone add this rule >

.thumb .thumbinner > div {
margin: 0 auto;
}

@TheDJ The centering is not working because of margin:1px. but if you add margin 0 auto to the thumbinner >div then it works but it also requires the float:none.

is the float and clear properties causing any other issues? not clear from task description.

the images should be one below the other and center aligned if the viewport is less than ~300

can someone add this rule [..]

That would require !important, which then would also change the margin on the thumb caption.

is the float and clear properties causing any other issues? not clear from task description.

Editors mostly complain about this:

the images should be one below the other and center aligned if the viewport is less than ~300

Ehm, Minerva currently does center aligning for everything under 768px (tablet cut off) if I remember correctly, I've never seen a ~300 value before in the stylesheets. As a matter of fact, this specific hack is never disabled above 768px, which it probably should...

this specific hack is never disabled above 768px, which it probably should...

i think it should be :) but as you said only above 768

Change 319474 had a related patch set uploaded (by TheDJ):
Rework Template:Multiple_image hack

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

TheDJ added a comment.EditedNov 2 2016, 10:05 PM

Right, i was thinking, since this is pretty much a mobile only issue, what if we solve this with flexbox ? Most mobile browsers will support that, the rest are a hideous mess already.

But flexbox allows us to do exactly what we want here, so it should be a perfect opportunity to use it here. Separate patch uploaded with my idea.

Tablet:


Just under tablet:

phone:

Collage on phone:

Change 319474 merged by jenkins-bot:
Rework Template:Multiple_image hack

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

Change 316551 abandoned by TheDJ:
Remove thumbnail style rule that is no longer required

Reason:
abandoned in favor of Ib6c1a4af34157384aded880c13078a647e342588

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

@Jdlrobson - where can we verify this?

It was merged last Thursday so it's riding the train and if we can find some test articles can be verified on Hebrew Wikipedia.
https://he.m.wikipedia.org/wiki/%D7%92%27%D7%95%D7%A8%D7%92%27_%D7%95%D7%95%D7%A7%D7%A8_%D7%91%D7%95%D7%A9

The specific problem it fixes can be tested on this page:
https://en.m.wikipedia.org/wiki/Los_Angeles_Lakers [note running the old code]
https://en.m.wikipedia.beta.wmflabs.org/wiki/Los_Angeles_Lakers [ running new code]

Before:

After:

@ovasileva let me know if you want to do any more testing or if you want to sign off.

ovasileva triaged this task as High priority.Nov 9 2016, 6:23 PM
ovasileva closed this task as Resolved.Nov 9 2016, 6:26 PM

all done!