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

Assigned To

Authored By

	Jdlrobson
	Jun 3 2020, 5:16 PM

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 params	Styles should be added??
?useskin=vector&useskinversion=2	NO
?useskin=vector&useskinversion=1	YES
?useskin=vector&useskinversion=2&useformat=mobile	NO
?useskin=vector&useskinversion=1&useformat=mobile	YES

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)

AC	Status	Details
1	✅	T254378#6292175

Details

Subject	Repo	Branch	Lines +/-
Simplify SkinVector::isLegacy(), add SkinVector::skinversion property.	mediawiki/skins/Vector	master	+17 -5
Drop wgVectorResponsive support from modern Vector	mediawiki/skins/Vector	master	+26 -33
Drop wgVectorResponsive support from modern Vector	mediawiki/skins/Vector	master	+19 -32

Customize query in gerrit

Related Objects

Mentioned In: T249363: Move the existing search to the header in preparation for Vue.js search development
Mentioned Here: T242772: Remove $wgVectorResponsive from legacy mode

Event Timeline

Jdlrobson created this task.Jun 3 2020, 5:16 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 3 2020, 5:16 PM

• nray awarded a token.Jun 3 2020, 5:16 PM

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

ovasileva set the point value for this task to 2.Jun 3 2020, 5:24 PM

ovasileva moved this task from Upcoming to Ready for Development on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jdlrobson updated the task description. (Show Details)Jun 9 2020, 5:42 PM

Jdlrobson mentioned this in T249363: Move the existing search to the header in preparation for Vue.js search development.Jun 10 2020, 6:21 PM

Volker_E awarded a token.Jun 10 2020, 6:44 PM

vas claimed this task.Jun 11 2020, 2:12 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

gerritbot added a project: Patch-For-Review.Jun 12 2020, 12:44 PM

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!

Jdlrobson reassigned this task from Jdlrobson to vas.Jun 12 2020, 4:20 PM

Jdlrobson moved this task from Ready for Development to Needs More Work on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jon, please review it again.

Jdlrobson moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.Jun 15 2020, 5:10 PM

Jdlrobson reassigned this task from Jdlrobson to vas.Jun 15 2020, 5:50 PM

Jdlrobson moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

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

We have to get rid enableResponsiveMode from initPage method because there is no longer enableResponsiveMode exists.

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.

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?

In T254378#6227880, @vas wrote:

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.

Jdlrobson reassigned this task from Jdlrobson to vas.Jun 16 2020, 8:28 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

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

Jdlrobson removed vas as the assignee of this task.Jun 26 2020, 6:47 PM

Jdlrobson moved this task from Needs More Work to Ready for Development on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jdlrobson edited projects, added patch-welcome; removed Patch-For-Review.

Peter.ovchyn claimed this task.Jun 29 2020, 11:15 AM

Peter.ovchyn removed a subscriber: vas.

Jdlrobson moved this task from Ready for Development to Doing on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.Jun 29 2020, 11:41 PM

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

gerritbot added a project: Patch-For-Review.Jun 30 2020, 1:53 PM

Jdlrobson moved this task from Doing to Needs More Work on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.Jul 1 2020, 4:05 PM

Peter.ovchyn moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.Jul 1 2020, 5:24 PM

There's a phan issue on the patch. Have left you feedback (https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/608625)

Peter.ovchyn updated the task description. (Show Details)Jul 2 2020, 12:56 PM

Peter.ovchyn updated the task description. (Show Details)Jul 2 2020, 12:59 PM

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.40; 2020-07-07).Jul 2 2020, 9:00 PM

Jdlrobson reassigned this task from Peter.ovchyn to Edtadros.Jul 2 2020, 9:08 PM

Jdlrobson moved this task from Needs More Work to Ready for Signoff on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jdlrobson moved this task from Ready for Signoff to Needs QA on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jdlrobson updated the task description. (Show Details)

Jdlrobson moved this task from Needs QA to Needs QA in Prod on the Web-Team-Backlog (Kanbanana-2019-20-Q4) board.

Jdlrobson added a subscriber: Peter.ovchyn.

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)

Edtadros updated the task description. (Show Details)Jul 9 2020, 4:35 AM

MBinder_WMF edited projects, added Web-Team-Backlog (Kanbanana-FY-2020-21); removed Web-Team-Backlog (Kanbanana-2019-20-Q4).Jul 9 2020, 5:13 PM

MBinder_WMF moved this task from Needs Analysis to Ready for Signoff on the Web-Team-Backlog (Kanbanana-FY-2020-21) board.Jul 9 2020, 5:13 PM

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

	F31921686: Screen Shot 2020-07-08 at 9.29.15 PM.png
	Jul 9 2020, 4:33 AM

	F31852336: Screen Shot 2020-06-03 at 10.15.42 AM.png
	Jun 3 2020, 5:16 PM

Drop wgVectorResponsive support from modern VectorClosed, ResolvedPublic2 Estimated Story PointsActions

Description

AC

Additional notes

Use cases:

QA steps

QA Results - Prod (TestWiki)

Details

Related Objects

Event Timeline

Test Result - Prod

QA steps

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