[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Page MenuHomePhabricator

Drop wgVectorResponsive support from modern Vector
Closed, ResolvedPublic2 Estimated Story Points

Description

Replication steps:

  • Add wgVectorResponsive = true in LocalSettings.php.
  • This adds an additional stylesheet to the skin. If you check the network tab you should see this (skins.vector.styles.legacy%2Cresponsive in the URL).
  • If you resize your browser window you will note the skin begins to exhibit some responsive behavior.
  • When you visit localhost:8888/?useskinversion=2 this stylesheet should not be added (we plan to replace this with something else)
  • When you visit localhost:8888/?useskinversion=1 this stylesheet should be added

Screen Shot 2020-06-03 at 10.15.42 AM.png (1×894 px, 836 KB)

AC

  • This config flag should be restricted to the legacy mode.

Additional notes

This is enabled on testwiki and causes some strange layouts on smaller layouts

Use cases:

Url paramsStyles should be added??
?useskin=vector&useskinversion=2NO
?useskin=vector&useskinversion=1YES
?useskin=vector&useskinversion=2&useformat=mobileNO
?useskin=vector&useskinversion=1&useformat=mobileYES

QA steps

Verify on https://test.wikipedia.org/wiki/Main_Page?useskinversion=2 that resizing the browser does not cause sidebar to overlap content.

QA Results - Prod (TestWiki)

ACStatusDetails
1T254378#6292175

Event Timeline

ovasileva triaged this task as Medium priority.Jun 3 2020, 5:20 PM

Change 605210 had a related patch set uploaded (by Vas Yaremchuk; owner: Vas Yaremchuk):
[mediawiki/skins/Vector@master] Drop wgVectorResponsive support from modern Vector

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

vas subscribed.

Hi Jon,
I made my patch, but I'm not 100% sure that my logic correct, it's not explained in the description
should we add that script if there is no get parameter 'useskinversion'.

Please let me know your thoughts.

Thanks!

Jon, please review it again.

Hi Jon,
please review it again,
I have followed your recommendations, but in that approach I can see following issues:

  1. We have to get rid enableResponsiveMode from initPage method because there is no longer enableResponsiveMode exists.
  1. The approach to use extension 'MobileFrontend' works only in the case if that extension exists, but if we don't have it, the approach will not work as it was before.
  1. If we would like to use protected method isLegacy() outside the class, we have to make it public. I'm not sure that it's a good idea. Thoughts?

Could we hope on a call to chat about that more?

  1. If we would like to use protected method isLegacy() outside the class, we have to make it public. I'm not sure that it's a good idea. Thoughts?

IMO it would be reasonable to make the skin version public api. Maybe not isLegacy() which could lose relevance in the far future, but getSkinVersion() instead.
In any case, seeing how many indirections are involved in calculating this value, I'd do this calculation only once, save the result in a property and return that from isLegacy() or getSkinVersion().

The isLegacy method is just a wrapper for the MediaWiki Service $isLatestSkinFeatureEnabled = MediaWikiServices::getInstance()->getService( Constants::SERVICE_FEATURE_MANAGER )->isFeatureEnabled( Constants::FEATURE_LATEST_SKIN ); - that's already a public API. No need to change the visibility of the existing method.

Change 605210 had a related patch set uploaded (by Vas Yaremchuk; owner: Vas Yaremchuk):
[mediawiki/skins/Vector@master] Drop wgVectorResponsive support from modern Vector

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

Jon, please review it again.

Thanks!

I'm assigned to the gerrit patch, so let's keep you assigned to this task from now on and track feedback on Gerrit. I've left you a new review.

Change 607024 had a related patch set uploaded (by Aron Manning; owner: Aron Manning):
[mediawiki/skins/Vector@master] Simplify SkinVector::isLegacy(), add SkinVector::skinversion property.

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

Change 605210 abandoned by Jdlrobson:
Drop wgVectorResponsive support from modern Vector

Reason:
Freeing up for somebody else to work on.

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

Change 608625 had a related patch set uploaded (by Peter.ovchyn; owner: Vas Yaremchuk):
[mediawiki/skins/Vector@master] Drop wgVectorResponsive support from modern Vector

https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/ /608625

Edtadros subscribed.

Test Result - Prod

Status: ✅ PASS
Environment: testwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

✅ AC1: Verify on https://test.wikipedia.org/wiki/Main_Page?useskinversion=2 that resizing the browser does not cause sidebar to overlap content.
The sidebar does not overlap when the window width is narrowed. The image below is at 500px. The menu and tabs do overlap some content.

Screen Shot 2020-07-08 at 9.29.15 PM.png (829×568 px, 215 KB)

Looks good, resolving.

Change 607024 abandoned by Jdlrobson:
[mediawiki/skins/Vector@master] Simplify SkinVector::isLegacy(), add SkinVector::skinversion property.

Reason:
This isLegacy function has now been removed from SkinVector so this is no longer applicable.

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