I agree with all the points.
Only nitpickingly I think that skin.less is aligned with other entrance point files like skin.json and therefore I'd prefer to remain with it.
And 'common' could be skins.vector.styles.common/ even though it's not an RL module. Otherwise I'd start thinking that common has everything and what's happening in skin.vector.styles/?

skins.vector.styles.legacy and skins.vector.styles share various files. For files used by both how will we arrange those? Putting them in skins.vector.styles.legacy seems preferable to ensure it's clear that we need to test in legacy when we change those files.

What about images shared by both but imported by different stylesheets.. will we duplicate those files or update the file paths in the stylesheets?

@Volker_E I like skins.vector.styles.common/ for consistency. I've updated the description with that, also including where layout folders could go.

In T264309#6510257, @Jdlrobson wrote:

skins.vector.styles.legacy and skins.vector.styles share various files. For files used by both how will we arrange those?

I propose creating a separate skins.vector.styles.common/ folder for files shared by both legacy and modern. Images used by both modes would go into skins.vector.styles.common/images/. However, if there are images used only in modern or legacy, the we can create a folder skins.vector.styles/images/ or skins.vector.styles.legacy/images/ and place the respective images there.

Regarding the entry files: skin.less and skin-legacy.less, I was thinking about naming both skin.less since they'd be in different folders. This is similar to the convention in Node of using index.js in multiple places. The downside of that however is that searching for those files in an IDE is difficult because they both have the same names (maybe not a big deal though). Another approach we could use for these entry files is to name them after their ResourceLoader module, so
skin.less → skins.vector.styles.less
skin-legacy.less → skins.vector.styles.legacy.less
That might be overthinking it though...

@Volker_E I like skins.vector.styles.common/ for consistency

This suggests that there would also be a ResourceLoaderModule called skins.vector.styles.common. I don't think we want to add additional ResourceLoader modules just for code organization.

Maybe we could indicate in the name that this is not a real module somehow and not add it to skin.json? I don't know what name to suggest. A leading "_" perhaps e.g. _common or maybe a generic src/ folder ?

skin.less → skins.vector.styles.less
skin-legacy.less → skins.vector.styles.legacy.less

FWIW I like this.

Sounds fair, @Jdlrobson. Documentation at https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader#Naming

In regards to our long-term idea of separating Vector modern and Vector legacy. Is adding one module really that performance critical?

I personally would say it's better to add the separate module regardless of performance. It makes it clearer what we're sharing between the two skins and makes it easier for us to know when that sharing is no longer useful.

Note T225842 would likely solve this at least for JS.

Looking at the link @Volker_E mentioned, https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader#File_organization

It is recommended that files ... do not reference files outside the folder. For example a file index.less inside folder resources/ext.foo.foo should not reference a file inside folder resources/ext.foo.bar. If this is happening please consider creating a new ResourceLoader module for shared resources and listing it as one of the modules dependencies.

I guess creating a ResourceLoader module for the sake of code organization could be justified, but I think that requires a bit more investigation.
For example, if we set the common less file as a dependency, and it imports a common variables file (@import "./variables";)

1. Will we have access to those variables from the dependant (modern/legacy) modules?
2. Can we override those variables in the dependant (modern/legacy) modules?

If the answer is yes to both those questions, then I don't mind making the common styles a real ResourceLoader module and setting it as a dependancy (its' just one module after all, right? 😛).

I don't see the variables thing as being a problem.
We import global variables from outside the resources folder (the root folder). We may even find this refactor cleans that up by reducing which variables are global.
For example font-size-tabs should not really be a global.

Change 661116 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/Vector@master] [WIP] Reorganizing Less files to match resourceLoader conventions

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

I've written a WIP patch just to see how this proposal shakes out (so I guess this could be considered an analysis).

It's a huge patch because it touches nearly every single file in the resource folder, along with the files for the storybook instance. There is some risk to that, and I'm
not sure it can be broken down into smaller chunks.

One issue that I've come across and haven't yet resolved it that the CSS background image URLs are no longer compatible with webpack & storybook. URLs start with background: url('images/...') are interpreted as ./images/... and this change breaks that directory structure.

I know this is the kind of thing webpack aliases were made for, but I haven't been able to get that to work yet.

The only small input I'd have is instead of /base/ folder putting the common files into the common root (1 up).

Move the common/ folder up one level & rename it to skins.vector.styles.common following ResourceLoader conventions.

The convention is to name it after the module. Naming it this way I think would be more confusing as looking at that, I would immediately suspect a needless module has been created. Naming it common/ would be clearer, I think, than naming it as a module that doesn't exist.

Oky doke. @Volker_E , @Krinkle , I've updated the description with your suggestions.

Looks reasonable to me. Is the variables.less Storybook issue under control with such structure. Guess we'll find out. :)

@Volker_E the Storybook (specifically Webpack) issue that I've run into so far is the background image URLs that start with url(image/ or img/. Webpack treats those as being a relative path, which would now be different (but not for resourceLoader, because RL just needs to compile the final entry file, which remains in the same place, relative to the images folder).

I thought the webpack aliasing feature would be able to take care of this, but I'm not sure at what level this resolving happens. Is it with the Less resolver, the Webpack URL resolver... still have to figure that out.

^The patch above reorganizes the CSS structure per the description. For the sake of clarity and ease of code-review, it does not touch the Storybook files in Vector. Instead, it creates aliases for them in the webpack config, and disables the url resolving in CSS (issue outlined in the comment above).

This will need a followup patch where the import paths in the Storybook stories are updated, and the Storybook instance is upgraded to work with Webpack 5. That version of Webpack allows us to use arrays as alias values, so we will be able to resolve the image urls to multiple paths:

{
"images": [ 
    path.resolve( __dirname, "../resources/common/images"), 
    path.resolve(__dirname, "../resources/skins.vector.styles/images") 
] }

That should fix the Webpack issue described above. 🤞

Test wiki created on Patch Demo by Phuedx (WMF) using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/39e736cccb/w/

We spotted a minor visual regression in the V2 footer layout on all special pages, which @Jdrewniak is working on:

Before	After	Diff

@alexhollender today shared in Slack to stay with currently deployed padding and revisit later if necessary.

Test wiki on Patch Demo by Phuedx (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/39e736cccb/w/

Test wiki created on Patch Demo by Phuedx (WMF) using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/1df1ce2499/w/

Change 677924 had a related patch set uploaded (by Phuedx; author: Jdrewniak):

[mediawiki/skins/Vector@master] Reorganize LESS files to match ResourceLoader conventions

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

Change 677924 abandoned by Phuedx:

[mediawiki/skins/Vector@master] Reorganize LESS files to match ResourceLoader conventions

Reason:

...

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

Test wiki on Patch Demo by Phuedx (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/1df1ce2499/w/

@Jdrewniak's patch has a +1 from both @Volker_E and myself. I've run a visual regression test on the patch against master using the top 10 enwiki pages by pageviews and all special pages and have found no differences. I'd appreciate someone else taking a look and +2ing.

A side note: I think it's worth pushing for context proxying from Wikipedias other than enwiki for Patch Demo.

I've investigated the visual regression identified by @phuedx , and after consulting with both @alexhollender and @nray, I think the footer padding in the patch related to this task is actually the correct one, which was mistakenly overridden previously and gone unnoticed.

The padding: 32px 0 0 0; value was based off of this design which was linked to this ticket T246420.

Originally this style was written inside a parent selector of .skin-vector-max-width . In this patch, that rule was moved into a different file without that selector, causing it to be overridden by a rule further down the cascade, in Footer.less.

The new patch however, changes the order of the imports, loading all of the common styles first, and the legacy/modern ones last, giving them priority.

Currently in skin.less you can see how the layout.less file, which contains the updated padding value for .mw-footer is loaded before Footer.less which contains the old value for the footer padding.

@Jdrewniak Makes all sense, I proposed to go forward with existing padding (not the overridden 32px top from this one design) in the patch after consulting with @alexhollender on Slack and keeping visual regressions and further design needs out of scope for the patch.
Note, that the patch is a) (greatly) done with above take from my POV b) blocking several sequential UI-specific patches as it makes no sense to propose and then go through such massive rebase.

Change 661116 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Reorganize LESS files to match ResourceLoader conventions

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

Put this into “Ready for Signoff” as @phuedx has already done a visual regression test on top articles and special pages. Seems fine to skip QA. Please move back if you disagree.