-
Notifications
You must be signed in to change notification settings - Fork 197
Fix @import statements with variables #33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix @import statements with variables #33
Conversation
|
No tests? |
|
This is good for compat in general, but I think our system would remain easier to reason about if we don't allow variable imports. I'm not aware of a use case for it currently in MediaWiki, and such constructs tends to make for hard-to-maintain or buggy code based on past experiences. If we accept this, I would recommend that initially our LESS linter would disallow use of it in MW-related code. |
Variable imports are uncommon, so far nobody felt the need for it. The very few cases (1 now, probably for ever) can be done safely with good documentation. The explicit nature of this import and simplicity of LESS makes it easier to understand and reason about than a custom (proprietary) solution in ResourceLoader that potentially requires reading pages of PHP code to find.
Good idea. I don't think there'll be ever more than one usage of this LESS feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Holding back for now. The skin variable project (for MediaWiki core) is not yet on the annual roadmap. When it is, this will be considered as part of that.
I don't think such delay is necessary or beneficial. It takes away from the momentum. |
|
Isn't this what is done with Twitter Bootstrap 3, especially with boot swatches? |
It could be used to parameterize which swatch to load. |
This is a fix for a bug (incompatibility with less.js) that would benefit third party users as well. After 3 months of nothing happening in these areas, do you wish to hold this up more, or can we consider making some actual improvement and doing some collaboration? |
Krinkle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs tests.
Feature documented at:
http://lesscss.org/features/#variables-feature-import-statements
Fixes:
#32
oyejorge#352
Merge from:
kylekatarnls@e3d4aa4
kylekatarnls@637fd93