Page MenuHomePhabricator

Vector should make use of deviceWidthTablet Less variable
Closed, ResolvedPublic

Description

The Vector skin currently uses 768px as a hardcoded value for a media query to instruct the browser to apply another set of stylesheet to the page for devices with a specific screen size. As MediaWiki provides a globally defined Less variable (@deviceWidthTablet) for that (when a device should be considered as a tablet), Vector should use this, instead of the media query in extension.json.

GCI task: https://codein.withgoogle.com/dashboard/tasks/5196097433632768/

Details

Related Gerrit Patches:

Event Timeline

Jdlrobson claimed this task.
Jdlrobson removed Jdlrobson as the assignee of this task.
Jdlrobson raised the priority of this task from to Normal.
Jdlrobson raised the priority of this task from Normal to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added projects: Vector, good first bug.
Jdlrobson set Security to None.
Jdlrobson added subscribers: gerritbot, DannyH, Florian and 3 others.
Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptJan 27 2016, 10:39 PM
phuedx triaged this task as Low priority.Jan 28 2016, 6:14 PM
sidak claimed this task.Mar 12 2016, 7:37 AM
sidak added a subscriber: sidak.

I found the hardcoded "768px" in the file skin,json
But I am not able to find any extension.js file in the Vector code (cloned from https://github.com/wikimedia/mediawiki-skins-Vector/ )

Further, do I need to define the variable for tablet threshold or is it already defined?

PS: I am beginner and would like to work on this bug.

Thanks

DannyH removed a subscriber: DannyH.Mar 14 2016, 5:05 PM

Hey @sidak apologies for missing this! Sorry for the late reply.
To fix this you'll need to look at skin.json and make the styles property of skins.vector.styles.responsive an array.
responsive.less should make use of the media query.
let me know if any other questions!

Jdlrobson updated the task description. (Show Details)Mar 17 2016, 9:51 PM
Isarra moved this task from Backlog to Bugs on the Vector board.Apr 7 2016, 1:13 AM
Isarra moved this task from Bugs to Implementation changes on the Vector board.Apr 7 2016, 1:52 AM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 17 2016, 6:31 PM
Jdlrobson removed sidak as the assignee of this task.Nov 8 2016, 5:38 PM
Jdlrobson added a project: Google-Code-In-2016.
Florian renamed this task from Vector should make use of deviceTabletSize LESS variable to Vector should make use of deviceWidthTablet LESS variable.Nov 12 2016, 2:33 PM
Florian updated the task description. (Show Details)
Florian moved this task from Proposed tasks to Imported in GCI Site on the Google-Code-In-2016 board.

I'll mentor this task for Google-Code-In 2016 :)

Florian updated the task description. (Show Details)Nov 12 2016, 2:35 PM
jo12bar claimed this task.Dec 12 2016, 1:48 AM
jo12bar added a subscriber: jo12bar.

Claimed as part of Google Code-In 2016 :)

Where exactly can I find documentation relating to this change? From previous comments, I believe that all I need to do is change the "skins.vector.styles.responsive" key in skin.json to be an array - but an array of what?

Please reply as soon as possible - I'm leaving on vacation in a few days, and the task expires in 4 days.

Thanks!

Alright, I've got an issue. I took this block in skin.json:

// ...
"skins.vector.styles.responsive": {
  "targets": [ "desktop", "mobile" ],
  "position": "top",
  "styles": {
    "responsive.less": {
      "media": "screen and (max-width: 768px)"
    }
  }
},
// ...

... and replaced it with this:

// ...
"skins.vector.styles.responsive": {
  "targets": [ "desktop", "mobile" ],
  "position": "top",
  "styles": {
    "responsive.less": {
      "media": "screen and (max-width: @deviceWidthTablet)"
    }
  }
},
// ...

However, this made it so that responsive.less wasn't being loaded at all. It doesn't throw any errors in the logs as far as I can see. However, it works properly if I just leave the media query in skin.json blank, like:

"media": {}

... And manually add the media query to responsive.less, like:

/*
  The styles below essentially place the navigation menu below the content,
  instead of to the side of it. They also hide the logo, as there's no space
  left for it.
*/
@media screen and (max-width: @deviceWidthTablet){
  div#mw-head {
    position: static !important; /* stylelint-disable-line declaration-no-important */
    margin-top: 0.5em;
  }
  // ...

This feels really dirty though. Because it completely bypasses ResourceLoader. I'm not sure what's causing this. Should I just go ahead with this approach?

Change 326385 had a related patch set uploaded (by Jo12bar):
Use global variable for tablet media query in responsive.less

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

Yes I think it's fine to move the media query logic into the less file itself and include it like a normal file - the end result is the same and all ResourceLoader does here is try to make this easier.

responsive.less would then just be an array value (not a key).

Volker_E renamed this task from Vector should make use of deviceWidthTablet LESS variable to Vector should make use of deviceWidthTablet Less variable.Dec 12 2016, 4:37 PM
Volker_E updated the task description. (Show Details)
Volker_E awarded a token.
Volker_E added a subscriber: Volker_E.

Alright, I'll switch it to an array when I get home from school - probably after 5:00pm.

Change 326874 had a related patch set uploaded (by Jo12bar):
Fix media query bug & change skin.json structure

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

Should be fixed :)

Alright, I just squashed both of those reviews into the original code review. Sorry that I submitted multiple reviews - I guess I'm too used to the GitHub workflow!

Thanks!

Just a heads up: I'm flying down to Mexico for a family vacation within the next two hours, and I probably won't have WiFi. As a result, I'll probably be unable to work on this bug until Christmas Day. Sorry about that. If I happen to find a WiFi hotspot, then I'll definitely check in, but otherwise I'll see you on the 25th!

Again, sorry about this!

Change 326874 abandoned by Jdlrobson:
Fix media query bug & change skin.json structure

Reason:
This was squashed into the parent commit.

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

Change 326385 merged by jenkins-bot:
Use global variable for tablet media query in responsive.less

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

Jdlrobson closed this task as Resolved.Dec 14 2016, 12:53 AM

Thanks @jo12bar ! All good!

sidak removed a subscriber: sidak.Dec 14 2016, 12:53 AM