Page MenuHomePhabricator

mobile-html: adjust page margins/padding
Closed, ResolvedPublic

Description

The content of the new mobile-html endpoint needs some left/right padding. We should also allow for padding or margin overrides through a JS function in the wikimedia-page-library. Native apps may want to add native elements on top or the bottom of the page.

Event Timeline

bearND triaged this task as Normal priority.Aug 7 2018, 4:10 AM
bearND created this task.

Content padding comes for free when using the vector skin (https://github.com/wikimedia/mediawiki-skins-Vector/blob/f010a2d8b8d90fb71c3ca9d8410b6c3db0ef36cd/components/common.less#L26), as shown in the /page/html endpoint output, but once again we're kind of on our own when it comes to using Parsoid content with the Minerva skin. IMO it would be best to add an equivalent rule to https://github.com/wikimedia/mediawiki-skins-MinervaNeue rather than putting a custom hack somewhere. In general I'd like to see Minerva become more Parsoid-aware, and it's probably up to us to make that happen.

Change 460220 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] WIP: Add skins.minerva.tablet.styles to base CSS modules

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

It might not be necessary (or desirable) to add a rule; the skins.minerva.tablet.styles module provides responsive content margins for the Minerva skin. It isn't currently in the base CSS modules but seems like it should be. @bearND is the WIP patch above something like what you had in mind for the standard padding/margins part of this?

I wonder how the Android app is setting margins currently, and whether they would be interested in leveraging the responsive design work from upstream through this module.

@Mholloway Thanks for checking this out.

The Android app sets margins in sections.js to values defined in the various values*/dimens.xml files for the various screen dimensions:

<!-- Default screen margins, per the Android Design guidelines. -->
<dimen name="activity_horizontal_margin">16dp</dimen>
<dimen name="activity_vertical_margin">16dp</dimen>

We need to be careful when making changes to the base CSS like this since this is now used by the Android app to generate the CSS bundles. If we rollback the change in the make-css-assets script in the Android app to use the base CSS endpoint then we'd have more flexibility until the mobile-html endpoint is established.

In general I like this idea. It would need some buy in from the affected teams, esp. design, since it does change the horizontal content margins. In my preliminary testing I still see the need to set a top margin for the apps.
Here are some example screenshot of tablet in landscape mode which shows the differences better:

Before:

After applying your patch (but with still applying margins via JS in the app):

After applying your patch (without applying any margins via JS in the app):
The top margin is definitely missing here. One could argue that the horizontal margins are needed as well.

Aklapper changed the edit policy from "Custom Policy" to "All Users".Sep 17 2018, 5:37 PM
Aklapper changed Risk Rating from N/A to default.

Change 460220 abandoned by Mholloway:
WIP: Add skins.minerva.[base,tablet].styles to base CSS modules

Reason:
Premature without more discussion/buy-in from the app teams.

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

Mholloway removed Mholloway as the assignee of this task.Sep 19 2018, 5:22 AM
Mholloway added a subscriber: Mholloway.

Not sure where to go with this without some more discussion.

Jhernandez added a comment.EditedSep 21 2018, 12:56 PM

Ill be discussing this with Nirzar to pick his brain and see if he thinks we should reach out to the designers to find some sort of common ground for all platforms.

The apps need the ability to set the margin width explicitly in px via JS rather than any additional upstream tablet CSS. Similar to setting the theme in T218049 the interface would ideally be wmf.setMarginWidth(width, callback). This would adjust the left and right margin of the content.

@JoeWalsh Do you also need to be able to set the top margin, e.g. for the lead image?
If so, should that all be in the same call, like a wmf.setMargins({top, right, bottom, left}, callback)?
Or would you prefer a separate call for top and bottom instead?
(Undefined values in the value object would be ignored, of course.)

@bearND yeah, good point - it should include top & bottom margins in the same call

bearND claimed this task.Mar 25 2019, 5:01 PM
bearND moved this task from To Do to Doing on the Product-Infrastructure-Team-Backlog (Kanban) board.

Released in page library 6.4.0. Still needs to be picked up by a new MCS deploy.

Mentioned in SAL (#wikimedia-operations) [2019-04-23T17:22:08Z] <bsitzmann@deploy1001> Started deploy [mobileapps/deploy@78985fb]: Update mobileapps to 6d3a422 (T201382 T217837)

Mentioned in SAL (#wikimedia-operations) [2019-04-23T17:26:14Z] <bsitzmann@deploy1001> Finished deploy [mobileapps/deploy@78985fb]: Update mobileapps to 6d3a422 (T201382 T217837) (duration: 04m 06s)

The margins and padding of the body can be adjusted by clients with the following calls on any mobile-html page now. Examples:

pagelib.BodySpacingTransform.setMargins(document.body, { top: '160px', right: '32px', bottom: '64px', left: '32px' })
pagelib.BodySpacingTransform.setPadding(document.body, { top: '160px', right: '32px', bottom: '64px', left: '32px' })
bearND closed this task as Resolved.Apr 23 2019, 6:12 PM