Page MenuHomePhabricator

[Bug] Special:MobileOptions heading padding should be consistent
Closed, ResolvedPublic2 Story Points

Description

Steps to Reproduce

  1. Visit http://readers-web-master.wmflabs.org/wiki/Special:MobileOptions
  2. Resize your browser to >= 720 px wide
  3. Observe that there's no whitespace below the Reading preferences tagline
  4. Resize your browser to < 720 px wide
  5. Observe that there's whitespace below the Reading preferences tagline

Expected Results

  • The whitespace below the Reading preferences tagline should always be there.

Actual Results

  • See the "Steps to Reproduce" section above and the following:
WidthResult
719 px
720 px

Environments Observed

Testing Environment for QA

Browser Version

  • Chrome (68.0.3440.106)
  • Firefox (61.0.2)

OS Version

  • macOS High Sierra (10.13.6)

Developer notes

.ns-special .heading-holder .tagline has a margin-bottom: 15px; not a padding-bottom.
Switching to a padding, you'll also need to update the rule for .ns-special #content .pre-content to introduce the margin needed for mobile display.

Event Timeline

phuedx created this task.Aug 22 2018, 5:01 PM
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptAug 22 2018, 5:01 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
phuedx added a project: Mobile.
phuedx updated the task description. (Show Details)Aug 23 2018, 9:37 AM
Jdlrobson moved this task from Needs triage to Triaged on the Mobile board.Aug 23 2018, 11:13 PM
ovasileva set the point value for this task to 2.Aug 29 2018, 4:47 PM

Will help mentor this in Google-Code-in-2018.

Change 472655 had a related patch set uploaded (by Takidelfin; owner: Takidelfin):
[mediawiki/extensions/MobileFrontend@master] Special/MobileOptions: Header padding consistency

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

Jdlrobson added a subscriber: Jdlrobson.

Hey @takidelfin thanks for your first contribution to mobile! Confusingly, the mobile site lives in two repos - MobileFrontend and Minerva. The latter will be needed for this fix!

Change 472701 had a related patch set uploaded (by Takidelfin; owner: Takidelfin):
[mediawiki/skins/MinervaNeue@master] Special/MobileOptions: Header padding consistency

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

>= 1000px>= 720px<720px

Does it behave as expected? @D3r1ck01 @Jdlrobson

Yup! In terms of the bug fix - you've got the right idea and those screenshots are what we're aiming for, but I've pointed out a few problems on the ticket with other special pages:


There's also a few suggestions there for your 2nd round of implementation! good luck!

takidelfin added a comment.EditedNov 9 2018, 8:41 PM

Hmm... I have used

!important

because of mobile.special.styles overriding mobile.mobileoptions.styles :/

Is there any way to fix this? Because then I can't set padding correctly. @Jdlrobson @D3r1ck01

Is there any way to fix this? Because then I can't set padding correctly

A technique that might be worth exploring is adding a bottom border to .ns-special .pre-content e.g. border-bottom: solid 1px transparent;
With this, maybe the margin-top can be added to the element after pre-content ?
Reading up on http://seifi.org/css/understanding-taming-collapsing-margins-in-css.html before your next attempt might give you some context with why this bug is happening!

takidelfin added a comment.EditedNov 9 2018, 9:03 PM
Expected behaviorActual behavior

Actually, I mean overriding padding: 0

@Jdlrobson

Okay, I found a way how to achieve it without !important it:

#section_0 {
	padding: 34px 16px 0;
}
.tagline {
	padding: 0 16px;
}

Change 472655 abandoned by Takidelfin:
Special/MobileOptions: Header padding consistency

Reason:
This issue is caused by MinervaNeue skin

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

>=1000px>=720px<720px

@Jdlrobson

@D3r1ck01 @Jdlrobson Could you please review my patch? Jenkins added +2, all tests have passed successfully. :D

@takidelfin the display is correct on mobile but it should look like this on tablet and desktop (>=720px):

umm, so that padding is expected? But I did it previously D:

This is what I see;

yep, there is 1 px padding :/
I really don't understand what is the expected behavior. Do you want consistent padding on all views or different for larger than 720px?

>=1000px>=720px<720px

@Jdlrobson
@D3r1ck01

Is this expected behavior?

No. The padding is different on mobile.

Mobile needs to look like:


(>=720px) like this:

umm.... but that is how it looks actually.

@Jdlrobson, that means the actual behavior on the task description is not a bug right? Because that is the actual behavior right now. But @phuedx expects that the padding should be there even in mobile or are my missing the point here? Thanks :)

The bug only exists with tablet. The issue is the paddi ng under reading preferences. Mobile should stay as it is.

Currently:


Required:

Ah! I thought that issue is with that spacing! Sorry ;(
I have added that 'internal padding' though.

@Jdlrobson Could you look at my newest patch? I have added a comment describing the purpose of max-width media query

Change 473140 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] [Bug] Special:MobileOptions heading padding should be consistent

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

Okay, so I had a deep dive into this while reviewing @takidelfin's patch. It gave me the idea of using a transparent border bottom to fix the collapsed margin. Unless I'm mistaken https://gerrit.wikimedia.org/r/473140 also solves this issue without the media query in https://gerrit.wikimedia.org/r/#/c/472701/. Both solutions solve the issue, so we can go with either. Am looking for someone to help me make that decision! Thanks in advance!

I've approved @takidelfin 's GCI task while we make this decision, so no rush!

Change 472701 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Special:MobileOptions: Consistent padding of heading

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

Jdlrobson removed takidelfin as the assignee of this task.Nov 13 2018, 10:58 PM
Jdlrobson added a subscriber: takidelfin.

Change 473140 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] [Bug] Special:MobileOptions heading padding should be consistent

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

ovasileva closed this task as Resolved.Nov 27 2018, 6:42 PM