| Fomafix | |
| Mar 12 2020, 4:25 PM |
| F31698756: Screen Shot 2020-03-23 at 2.11.17 AM.png | |
| Mar 23 2020, 9:14 AM |
| F31698757: Screen Shot 2020-03-23 at 2.10.58 AM.png | |
| Mar 23 2020, 9:14 AM |
| F31678106: grafik.png | |
| Mar 12 2020, 4:25 PM |
| F31678104: grafik.png | |
| Mar 12 2020, 4:25 PM |
Since rSVECac069fbec122: [dev] Consolidate ResourceLoader LESS style files the border color of Vector action menu is black instead of gray.
Steps to Reproduce:
Actual Results:
Expected Results:
| AC | Status | Details |
|---|---|---|
| 1 | ✅ | T247537#5991301 |
Thank you for identifying this @Fomafix and reporting! My apologies for the trouble.
Hey @ovasileva! It seems I've introduced a regression so I'm going to look at this now.
Change 579325 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [fix] [LESS] Move print style variables to query
Quick status update: this fix is ready to go but is blocked on further technical refactor discussion. As I understand it, there are a few options:
I'm in tomorrow but out of office next week so I leave it to the team to decide.
I endorse the immediate fix. The 3rd resolution will add additional refactoring, thus risk and discussion. The immediate need is the fix, I think further technical debt reduction is best done in a follow-up patch.
Change 579603 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/Vector@master] Don't use variables for values only ever used once
The issue is fixed so dropping to medium.
I am not 100% happy with the solution as I think this pattern can lead to further issues if it was intentional.
So to me the conversation with @Volker_E is super important and this remains blocked until it happens.
I've raised https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/579603 to capture the work I think needs to happen.
From team chat:
Jon Robson Yesterday at 11:35 AM @volker_e a question about variables came up in code review with @stephen on https://gerrit.wikimedia.org/r/#/c/579325/1/resources/skins.vector.styles/print.less - my understanding is within a skin/repo we should never use the same variable name with different values more than once, right?? 5 replies Jon Robson 23 hours ago It seems Vector uses @color-base: #222; in variables.less but then defines another variable for print styles with the same name but different value @color-base: #000; Jon Robson 23 hours ago My feeling is the latter should be called print-color-base or something different? the dangers of clobbering names with different values is scope can be ambiguous
Change 579325 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [fix] [LESS] Move print style variables to query
My take has been that print styles are never concatenated or merged into screen styles and assuming clear scope of variables for performance reasons.
If you think we can't assure that, we need to go on a different path. But I'm not sure if we're are going to fix the symptoms, not the cause.
LESS compiler variable-scopes lies within @media rules. https://phabricator.wikimedia.org/rSVECac069fbec1222bcb196d3d33fe073b901748370f#change-N3Jq3vXFMwYg
I think that's sufficient. No print variable should be defined outside @media print.
What the other approach would lead to, is to redefine every single variable as -print variable as well, which seems counterproductive to me.
Let me be a bit clearer: One beauty of CSS (and LESS) variables is, that they can be contextually amended if necessary. If we define new ones, we're giving up this power.
As I understand you're saying:
@border-color-base within the @media print scope (pseudo-code: @media-print[@border-color-base]) will only apply for print media, thus it is enough distinction from the global @border-color-base which is inherited to @media screen.
These can be defined in variables.less by wrapping them into the @media print selector.
There's one benefit to this: the for print (darker) @border-color-base would be applied to normal (not print-specific) styles too in print if it was used (currently only in .menu which is hidden in print).
Sure but now every time I use a LESS variable I now have to be aware of the scope it runs in and can't assume it has the value defined globally.
Up until now https://github.com/wikimedia/mediawiki-skins-Vector/blob/master/variables.less#L39 was a definition of global variables with the expectation they wouldn't be overriden. Now print.less is importing this file and then changing the value of one of them. Now if I wanted to use
an import statement print and import variables (as it currently our practice so that storybook can import them) what would I expect the value of base-color to be? The global or local value? If I wanted the local value I'd need to set it again...
A compromise might be prefixing these variable names with local/context so the reader knows that their values are dependent on other things and moving color-base out of the variables.less file.
Change 579603 abandoned by Jdlrobson:
Don't use variables for values only ever used once
Reason:
I talked with Volker some more and unable to convince him. I respect his preference here to keep as is so I'll leave the status quo.
That said I'm still not convinced about the pattern of clobbering variables with different values and I will bring it up again if I see it causing further problems in the codebase. Not a priority right now though.
Status: ✅ PASS
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX
Test Artifact(s):
Visit https://en.wikipedia.beta.wmflabs.org/wiki/Main_Page
Hover over more
✅ AC1: Confirm the border is not black (compare and contrast with production if this is not clear)
| Production | Beta |
|---|---|