-
-
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
Header scss optimisation #3050
Header scss optimisation #3050
Conversation
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@@ -1,3 +1,10 @@ | |||
/** | |||
* @copyright Copyright (c) 2017, John Molakvoæ (skjnldsv@protonmail.com) |
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.
We also need to include the original authors of the original file ;)
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.
I included the most active ones.
There is 16, should I include all of them? Or should we remove the copyright header entirely?
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 about most active ones: it's about actual contributors to this file
* @license GNU AGPL version 3 or any later version | ||
* | ||
*/ | ||
|
||
/* prevent ugly selection effect on accidental selection */ | ||
|
||
#header, #navigation, #expanddiv { |
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.
Code-style wise these should be on separate lines, one per selector.
@@ -25,7 +32,6 @@ | |||
} | |||
|
|||
/* HEADERS ------------------------------------------------------------------ */ | |||
|
|||
#body-user #header, #body-settings #header, #body-public #header { |
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 here, separate lines per selector for readability please. :)
@@ -39,7 +45,6 @@ | |||
} |
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.
Variables will come after @Espina2
Please see the main scss pr!
margin-top: -21px; | ||
padding: 0; | ||
vertical-align: middle; | ||
} | ||
} | ||
|
||
/* hover effect for app switcher label */ |
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.
In here instead of repeating the code you why you don't use @extend?
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.
What do you wish to extend exactly? There isn't that much similarity.
And extend is great amongst similar properties, but since we generate the css, we need to aim for a balance between readability and optimisation.
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@@ -1,20 +1,23 @@ | |||
/** | |||
* @copyright Copyright (c) 2017, John Molakvoæ (skjnldsv@protonmail.com) | |||
* @copyright Copyright (c) 2016, Lukas Reschke (lukas@statuscode.ch) | |||
* @copyright Copyright (c) 2016, Julius Härtl (jus@bitgrid.net) | |||
* @copyright Copyright (c) 2016, Jos Poortvliet (jospoortvliet@gmail.com) |
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.
@skjnldsv I wrote the original file in 2014, that’s what I meant ;)
https://github.com/nextcloud/server/blame/stable11/core/css/header.css
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.
I created a script to output the original copyrights from the commits.
I will create a special pr to put all the copyrights correctly :)
Can you review the css only? I will update all the copyrights on a different pr :)
By the way, this might conflict with #2924 by @ChristophWurst depending on what work is done further. Could you coordinate here @ChristophWurst @skjnldsv, maybe merging this one into #2924? |
@jancborchardt We should merge this one first since #2924 doesn't modify header.scss as much as this pr :) |
Current coverage is 53.99% (diff: 100%)@@ master #3050 diff @@
==========================================
Files 1302 1302
Lines 80061 80766 +705
Methods 7902 8006 +104
Messages 0 0
Branches 1245 1303 +58
==========================================
+ Hits 43170 43611 +441
- Misses 36891 37154 +263
- Partials 0 1 +1
|
@ChristophWurst can you also check this then to make sure it kinda is in line with what you are doing? Or that there’s no duplicate work or conflicts? |
I'd way we merge mine first then @skjnldsv has to do the merge conflict resolution fun 😉 |
Please don't! 😭 |
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.
Clicked a bit around and looks good 👍
Sorry @ChristophWurst 😉 |
Optimisation and simplification of the header.scss file
@nextcloud/designers