Page MenuHomePhabricator

Avoid ResourceLoaderGetLessVars hook in MobileFrontend by adding mobile threshold to Less variables in core (MFDeviceWidthMobileSmall)
Closed, ResolvedPublic

Description

MobileFrontend currently uses this hook to register three one global Less variables.

Use of this hook is deprecated because of inherent cache problems, as well as due to tendency to create slippery dependencies between extensions.

  • The values of these variables should be maintained elsewhere. (It seems this is done already, the values from from constants and configuration variables).
  • Should be exposed to individual modules's stylesheets by using a getLessVars method. The easiest way to do this is through a Module subclass. This could be generalised if needed.

Usage

  • wgMFDeviceWidthMobileSmall – comes from $config->get( 'MFDeviceWidthMobileSmall' )
    • Used in MobileFrontend
    • Used in MinervaNeue.

The use in MinervaNeue presumably means there is already a dependency on MobileFrontend from that skin. If so, then it could potentially use the same Module subclass as the one MobileFrontend will use. Alternatively, it could have its own getLessVars method to register the same less variable from the same source. (Thus making Config the API, instead of global LessVars itself).

Event Timeline

Change 366993 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/MobileFrontend@master] Hiegyne: Remove unused global LESS vars wgMFThumbnailTiny and wgMFThumbnailSmall

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

Jdlrobson added a subscriber: Volker_E.

@Volker_E can we defined this in wikimedia-ui or mediawiki-ui's variables...?

@Jdlrobson We potentially could, but WikimediaUI Base is meant to be a place for variables used across Foundational(/Movement) products. If this is just used in MobileFrontend, why not putting it there? Any plans to extend its usage?

It should be used cross movement.
The idea is to define thresholds centrally.

We already have 720px as the threshold for tablet. This would be the threshold from feature to smartphone on mobile (we should also have a threshold for desktop monitor...)

Change 366993 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Remove unused global LESS vars wgMFThumbnailTiny and wgMFThumbnailSmall

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

Sorry, I was looking at the removed vars for a moment and didn't look at the remaining. YES!
That has been on my mind for quite a while and I think that it is more than useful to put mobile breakpoints into WikimediaUI Base.

I'm pretty sure @TheDJ has filed a task quite a while ago about exactly this, there is just none of his open ones the one I seemingly remember. There has been some discussion about widescreen breakpoints at T88114#1135104

@Volker_E We discussed it as one of the 'problems' for TemplateStyles (how to make sure all template use the same cut off values) and also a bit for my responsiveContent gadget. Not sure if there is a specific ticket related to this though.

P.S. not entirely sure it needs to be uniform across skins. I actually doubt it. It could just be that the variable is named the same for each skin, but actual value differs.

@TheDJ From my experience it doesn't make sense to have slightly different breakpoints for different products as it puts extra burden on designers and developers and the main reasoning behind should be device support, not design.
Moreover WikimediaUI Base vars are still not fixed values, but act as base to lift solution finding task from devs, which they still could overwrite easily.

Jdlrobson renamed this task from Avoid ResourceLoaderGetLessVars hook in MobileFrontend to Avoid ResourceLoaderGetLessVars hook in MobileFrontend by adding mobile threshold to LESS variables in core (MFDeviceWidthMobileSmall).Aug 8 2017, 3:49 PM
Jdlrobson moved this task from Untriaged to Move to Backlog on the Web-Team-Backlog (Tracking) board.
Volker_E renamed this task from Avoid ResourceLoaderGetLessVars hook in MobileFrontend by adding mobile threshold to LESS variables in core (MFDeviceWidthMobileSmall) to Avoid ResourceLoaderGetLessVars hook in MobileFrontend by adding mobile threshold to Less variables in core (MFDeviceWidthMobileSmall).Aug 8 2017, 5:05 PM
Volker_E updated the task description. (Show Details)

Change 372551 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Add mobile and desktop thresholds

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

Change 381253 had a related patch set uploaded (by VolkerE; owner: VolkerE):
[wikimedia-ui-base@master] Add mobile and desktop thresholds

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

Change 381253 merged by jenkins-bot:
[wikimedia-ui-base@master] Add mobile and desktop thresholds

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

Ping @Volker_E - do you have an alternative patch to https://gerrit.wikimedia.org/r/372551 or can that be merged?

@Volker_E It is marked as blocked, but not entirely sure why. If the reason was the mentioned increase in maintenance by having the variables in two places (wikimedia-ui and mediawiki-core), I'd like to ask reconsideration. At the moment wikimedia-ui is not yet being integrated or provided in core less context. Unless that is finished, we might not want to block on that. Also, this wouldn't increase the number of places we maintain this variable. They have been hardcoded in MobileFrontend for years.

This commit merely moves them away from MobileFrontend PHP code that uses deprecated ResourceLoader interfaces, to a common place in MediaWiki core where they can be re-used by other code through LESS imports. This seems like a step in the right direction.


Blocked on https://gerrit.wikimedia.org/r/372551

@Jdlrobson Replacement landed. Global var usage can now be replaced. Let me know once that is completed, so we can proceed with the parent task :)

Change 372551 merged by jenkins-bot:
[mediawiki/core@master] Add mobile and desktop thresholds

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

Change 408338 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Explicitly use LESS variable for breakpoint defined in mediawiki ui

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

Change 408338 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Explicitly use LESS variable for breakpoint defined in mediawiki ui

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

Jdlrobson claimed this task.

This appears to be done.

It seems the ResourceLoaderGetLessVars hook is still used by MobileFrontend in latest master.

https://gerrit.wikimedia.org/g/mediawiki/extensions/MobileFrontend/+/38753e355a5d6be52167c9898e2f3968d47cccef/includes/MobileFrontend.hooks.php#1019

In other news, the hook use in Minerva is now resolved :) – T171367.

Change 425132 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop unused hook and $wgMFDeviceWidthMobileSmall variable

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

Change 425132 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop unused hook and $wgMFDeviceWidthMobileSmall variable

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