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

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!

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

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

@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!

In T249372#6131618, @gerritbot wrote:

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?

In T249372#6147215, @Krinkle wrote:

In T249372#6131618, @gerritbot wrote:

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.

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.

In T249372#6153248, @Jdlrobson wrote:

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 […]

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

See:

• https://codesearch.wmflabs.org/search/?q=%5B%27%22%5D%5B%5E%27%22%5D*%5C.menu%5B%20.%3E%23%7B%2C%27%22%5D&i=nope&files=js%24&repos=
• https://codesearch.wmflabs.org/search/?q=%5C.menu%5B%20.%3E%23%7B%2C%5Cn%5D&i=nope&files=(css%7Cless)%24&repos=
• https://global-search.toolforge.org/?q=%28%5C.menu%5B+%3E%23%7B%2C%5Cn%5D%7C%5B%27%22%5D%5B%5E%27%22%5Cn%5D%5C.menu%5B+.%3E%23%2C%27%22%5D%29&regex=1&namespaces=2%2C4%2C8
  • At least 1,787 pages. Likely a bit more given this pattern is still overly restrictive to reduce false results.

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

@Jdlrobson I've noticed the "More" menu no longer can be pinned: it won't stay open when clicked, although the dropdown arrow rotates up. I think the checkbox hack's application broke there. I also think it is better this way 😄 (closing the menu was counter-intuitive), but probably someone will report it. If nobody reports it, it would indicate this functionality was not useful, anyway.

If it gets fixed, I suggest using focus loss as the trigger to close the menu. :focus-within is a pure-CSS solution to pin the dropdowns, except for... IE, which can be handled with JS.

Unfortunately I haven't had time to thoroughly review these changes, excuse me for raising these points after the merge.

Some of the classes introduced are coupled to the Vector skin, while denoting information that's universal to skins:

1. .emptyPortlet being replaced by .vector-menu-empty
2. .vector-menu[-*]
3. Possibly #p-personal -> .vector-menu-personal

On T252447 it is suggested to gadget authors to use these classes, like:
#p-personal > .vector-menu-content > ul

Gadgets are seldom specific to one skin, when adding menu items, the gadget authors would like that their tool works for all skins, without the need to add custom code for each supported skin. The latter is unnecessary burden and unmanageable, ex. if a new skin is introduced.

I don't see any benefit to naming these classes vector-* other than namespacing, which could be mw-* as well. Drawbacks, however, will become more apparent as time passes. We can already see bad practices developing: https://commons.wikimedia.org/w/index.php?title=User:Geagea/adminwatch.js&diff=422834081&oldid=227940368
(note: I've tried to correct that in T252447#6179679, I'm not sure that will succeed...).

.vector-menu-empty has also caused trouble: T253912, T253819 (because of caching, not the name though). .emptyPortlet is misleading and non-compliant with the naming convention, so it's reasonable to rename, but coupling it to Vector only adds complexity (patch) without added functionality. I'd suggest using .mw-menu-empty and deprecating .emptyPortlet (addPortletLink in core needs to remove both until all skins migrate).

Gadgets should really be creating their own classes on elements they create and if needed using the reliable universal selectors such as #p-personal.

We have a duty here to help gadget developers improge their gadgets.

The 'vector-' prefixed classes are vector internals and I think it's positive that those classes are unique to Vector. Obviously there are issues with a gadget developer using them but that's a social problem we'll need to address later on.

As for emptyPortlet let's continue that conversation in T253938 - you raise some good points but let's not fragment that conversation.

In T249372#6182143, @Jdlrobson wrote:

We have a duty here to help gadget developers improge their gadgets.
The 'vector-' prefixed classes are vector internals

I've removed these from T252447. Sharing internals with gadget devs is neither safe or helpful.

and I think it's positive that those classes are unique to Vector.

If it's not used outside Vector then it does not matter and I rest my case. However, we can't stop people using those. In fact, notifying gadget authors of these classes is the opposite of internal.
On a second thought: menus, dropdown menus are recurring constructs in skins. If you want a simple skinning system, where designers can focus on... design, not implementation details, then abstracting these constructs (intentionally not using "component": some patterns can be reused, not the whole) and reusing them in each skin will reduce the necessary effort to create a skin. This is where consistency pays off, this is the benefit of a common naming.

I remember your stance on consistency between skins. This is a different aspect than discussed a long time ago and I won't further comment on that.
However, I wonder what positives do you see in classes unique to Vector.

Obviously there are issues with a gadget developer using them but that's a social problem we'll need to address later on.

Social? Think about game theory: the rules of the game determine the outcome. You set the rules.

As for emptyPortlet let's continue that conversation in T253938 - you raise some good points but let's not fragment that conversation.

I'll replicate the relevant comment there.

This task was about simplifying the code in Vector not all skins. Now we have a simplified definition of all menus. Classes in MediaWiki reflect where where code lives. Since the code for these menus is created in Vector the classes are prefixed vector and unique to Vector.

On a second thought: menus, dropdown menus are recurring constructs in skins.

They are but right now they are not built that way. They are cargo cult programmed. The only consistency in menus is the ID. In future I hope to have a function in Skin.php in core that encapsulates all these rules but there's much more to do before we can get to that point.

You set the rules.

We do set rules but they need to be enforced socially. We have mw.util.addPortletLink for adding menus. We can't enforce rules - we allow editors to run arbitrary JS and CSS so the best we can do is set expectations and guidance. That's not going to happen over night and is not going to be without pain.

In T249372#6182374, @Jdlrobson wrote:

...

For me these are ideological, not technical arguments: no use-case that shows benefit.