-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Implement scss variables #3530
Implement scss variables #3530
Conversation
@juliushaertl, thanks for your PR! By analyzing the history of the files in this pull request, we identified @skjnldsv, @ChristophWurst and @rullzer to be potential reviewers. |
This is looking soooo goooood! 💟 |
Sorry if this is the wrong place, but I haven't found anything in the docs or in the respective issues, so: |
Great, thanks, I should have looked in the PRs too. |
Good point. @skjnldsv Can you add a note about this in the dev docs? |
lib/private/Template/SCSSCacher.php
Outdated
@@ -114,6 +113,27 @@ private function isCached($fileNameCSS, $fileNameSCSS, ISimpleFolder $folder, $p | |||
} | |||
|
|||
/** | |||
* Check if the variables file has changed | |||
* @param $fileNameCSS |
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.
string? potatoes? ananas? array? ;)
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.
🍍 👍
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.
Hmm now I'm hungry. 😮
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.
🍕
bbe3ef3
to
e3a621d
Compare
I've now replaced all the colors with temporary variables, so we can better track which color it was before. Next step would be to reduce those values to a basic set of variables: server/core/css/variables.scss Lines 1 to 10 in e3a621d
I'm not sure how we should handle the simplification of the massive amount of grey values: server/core/css/variables.scss Lines 21 to 43 in e3a621d
@skjnldsv Using darken/lighten function might be ok, but when it comes to theming (especially themes with inverted colors) using rgba($color-main-text, 0.1) for the different grey variations might be better. What do you think? |
Couldn't we set the darken/lighten functions into the variable.scss file? |
For the grey values, we should merge what we have into simpler ones. But this could require to look exactly where they are used since they may be erroneous already. I can see someone putting a #eee right next a #e6e6e6 without seeing what he was doing on the first place :p |
core/css/variables.scss
Outdated
$color-main-old-f8f8f8: #f8f8f8; | ||
$color-main-old-245: rgb(245,245,245); | ||
$color-main-old-f0f0f0: f0f0f0; | ||
$color-main-old-238: rgb(238, 238, 238); |
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.
Same as #eee
core/css/variables.scss
Outdated
$color-main-old-fafafa: #fafafa; | ||
$color-main-old-f8f8f8: #f8f8f8; | ||
$color-main-old-245: rgb(245,245,245); | ||
$color-main-old-f0f0f0: f0f0f0; |
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.
Missing #
?
core/css/variables.scss
Outdated
$color-main-old-eeeeee: #eee; | ||
$color-main-old-e8e8e8: #e8e8e8; | ||
$color-main-old-dddddd: #ddd; | ||
$color-main-old-220: rgb(220, 220, 200); |
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.
It's not really grey, but slightly green #DCDCC8
- But I can't find any reference in the above code.
core/css/variables.scss
Outdated
$color-main-old-220: rgb(220, 220, 200); | ||
$color-main-old-d3d3d3: #d3d3d3; | ||
$color-main-old-cccccc: #ccc; | ||
$color-main-old-187: rgb(198, 198, 198); |
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.
#C6C6C6
core/css/variables.scss
Outdated
$color-main-old-d3d3d3: #d3d3d3; | ||
$color-main-old-cccccc: #ccc; | ||
$color-main-old-187: rgb(198, 198, 198); | ||
$color-main-old-190: rgb(190, 190, 190); |
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.
#BEBEBE
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.
🐑
core/css/variables.scss
Outdated
$color-main-old-bbbbbb: #bbb; | ||
$color-main-old-aaaaaa: #aaa; | ||
$color-main-old-999999: #999; | ||
$color-main-old-150: rgb(150, 150, 150); |
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.
#969696
core/css/variables.scss
Outdated
$color-main-old-666666: #666; | ||
$color-main-old-555555: #555; | ||
$color-main-old-222222: #222; | ||
$color-main-old-333333: #333333; |
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.
Sorted list of grey values:
#222222
#333333
#555555
#666666
#777777
#888888
#969696
#999999
#aaaaaa
#bbbbbb
#bebebe
#c6c6c6
#cccccc
#d3d3d3
#dddddd
#e8e8e8
#eeeeee
#eeeeee
#f0f0f0
#f5f5f5
#f8f8f8
#fafafa
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.
e3a621d
to
1efaf0d
Compare
b599947
to
efaefd2
Compare
efaefd2
to
82bbd02
Compare
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
…ackground Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Haertl <jus@bitgrid.net>
f9fd812
to
1ebec0d
Compare
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
e6b6add
to
53c0fe6
Compare
Codecov Report
@@ Coverage Diff @@
## master #3530 +/- ##
===========================================
+ Coverage 54.23% 54.3% +0.07%
Complexity 21148 21148
===========================================
Files 1304 1304
Lines 80779 80782 +3
Branches 1282 1282
===========================================
+ Hits 43810 43869 +59
+ Misses 36969 36913 -56
Continue to review full report at Codecov.
|
@MorrisJobke @LukasReschke @skjnldsv @nextcloud/designers Ready for review. We can squash those commits, but I think it might be easier to trace what has changed exactly for reviewing. |
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.
Tested and works nicely 👍
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.
Works nicely :)
🎉 Thanks a lot @juliushaertl |
This is a basic implementation for using SCSS variables. For now I only intruduced variables for colors. We can add others for border-radius, etc. later.
fix #2288 #3256 #3211
@skjnldsv @nextcloud/designers Please have a look if it makes sense so far or if anything important is missing.
Applied to files