Page MenuHomePhabricator

[Dev] DRY up the menu templating code
Open, HighPublic3 Estimated Story Points

Description

VectorTemplate has various methods with very similar logic. These are buildNamespacesProps, buildVariantsProps, buildViewsProps, buildActionsProps and buildPersonalProps

I suggest we DRY this up into a single method called getMenuData and append any additional data where needed to make it obvious how these different menus differ.

Event Timeline

Jdlrobson created this task.Apr 3 2020, 8:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 3 2020, 8:37 PM

Change 585808 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] DRY up menu creation!

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

ovasileva triaged this task as Medium priority.Apr 6 2020, 5:21 PM
ovasileva set the point value for this task to 3.Apr 6 2020, 5:24 PM

Change 585801 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Standardize menu data (DRY!)

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

Change 586438 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] One menu template to rule them all

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

Okay the refactor patches are up. I'm hoping this will be the last time we need to touch legacy Vector. I've tried to simplify the components legacy Vector needs so we can park its templates in its own folder (potentially includes/templates/legacy) and rely on Typescript to help us not break those contracts going forward. I'll switch gears to the header work while I await your reviews. Thanks in advance!

Demian added a subscriber: Demian.Apr 8 2020, 3:58 AM

Change 587536 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Portal is also a VectorMenu

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

ovasileva raised the priority of this task from Medium to High.Apr 13 2020, 5:09 PM

Generally, the code looks good and the refactorings are very very welcome, but I didn't have the time to test them yet. Maybe someone who can actually +2...

Change 585801 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Standardize menu data (DRY!)

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

Change 591512 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Make VectorMenu template data conform with MenuDefinition

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

Change 591513 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Make VectorTabs template data conform with MenuDefinition

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

Change 591512 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Make VectorMenu template data conform with MenuDefinition

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

Change 591513 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Make VectorTabs template data conform with MenuDefinition

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

Change 585808 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: DRY up menu creation!

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

Change 589395 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Simplify and standardize menu definitions

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

Change 594601 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Generalise personal menu

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

Change 594761 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Refactor: Merge VectorTabs into Menu

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

Change 594601 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Generalise personal menu

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

Change 594761 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Merge VectorTabs into Menu

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

Change 586438 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: VectorMenu merged in to Menu

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

Change 587536 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Portal is also a Menu

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

This refactoring causes the variant drop-down menu in vector skin no longer contains all language variant options. Please see T249372 for detail. I strongly suggest reverting the deletion of function buildVariantsProps which is deleted in patch 585808 as this affect ALL Wikimedia Projects that utilize LanguageConverter.

Change 595160 had a related patch set uploaded (by VulpesVulpes825; owner: VulpesVulpes825):
[mediawiki/skins/Vector@master] Fix: Adding back previous language varaint code before refactor

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

Change 595160 abandoned by VulpesVulpes825:
Fix: Adding back previous language varaint code before refactor

Reason:
Waiting for simpler solution or a full revert.

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

Change 595198 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Minor: Remove unused Portal.mustache

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

Change 595198 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Minor: Remove unused Portal.mustache

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

Change 595662 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] BaseTemplate:makeListItem is deprecated

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

Change 589395 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Simplify and standardize menu definitions

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

Jdlrobson added subscribers: Jdrewniak, Niedzielski.

@Niedzielski or @Jdrewniak would you be up for reviewing the cleaner menu template code and signing off. Feel free to document future opportunities in new tasks if there's anything that doesn't make sense to you!

Jdlrobson moved this task from Needs triage to Technical on the Vector board.Fri, May 15, 4:58 PM
Krinkle added a subscriber: Krinkle.EditedTue, May 19, 12:59 AM

Change 589395 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Simplify and standardize menu definitions

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

This causes the deprecated class="menu" reserved for Legacy Vectors dropdown menus to become applied to all sidebar portlets, p-personal and tab areas in Legacy Vector and New Vector. Those seem like unintended side-effects and are potentially breaking changes for users. Also, by applying that to New Vector it would become rather difficult for gadgets to test scripts as we go along, as they would not have the confidence that something is compatble. Could this class be limited to a condition only enabled for Legacy Vector's dropdown menus?

Change 589395 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Refactor: Simplify and standardize menu definitions

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

This causes the deprecated class="menu" reserved for Legacy Vectors dropdown menus to become applied to all sidebar portlets, p-personal and tab areas in Legacy Vector and New Vector. Those seem like unintended side-effects and are potentially breaking changes for users. Also, by applying that to New Vector it would become rather difficult for gadgets to test scripts as we go along, as they would not have the confidence that something is compatble. Could this class be limited to a condition only enabled for Legacy Vector's dropdown menus?

A conditional could be added but it adds more complexity to the template code so it's a trade off I'd rather avoid.

How do we feel about removing the menu class altogether with a user notice?

https://codesearch.wmflabs.org/search/?q=%5C.menu%20%2B&i=nope&files=.*(css%7Cless)&repos=
https://global-search.toolforge.org/?q=%22.menu+%22&namespaces=2%2C4%2C8 shows about 447 pages from 312 users and 40 site css pages. This seems a small enough sample size to warrant dropping the class.

Krinkle added a comment.EditedWed, May 20, 6:36 PM

Let's add the conditional and deal with this as part of the general New Vector transition. It's not worth the cost. This is becoming rather disruptive to volunteers. These should not be undertaken to slightly. For the most part these messages are a dead-end. This stuff takes time.

I'm also unable to help the way I usually do as a volunteer because I'm much busier then usual at WMF, and personally with other things. There's various blocking issues (T236828, T9356), and most of the volunteers I collab with have also been absent or unavailable to help during these "unprecedented" times.

Incomplete regexes, and excludes JS which is arguably where it is most impactful in terms of hard breaking without fallback.

See:

pages from 312 users

As far as I'm concerned, this is not about personal style preferences. It is about gadgets, site scripts, and shared user scripts.

I should note, I've not looked into the complexity of adding a conditional. Let's continue this conversation in T253329