Header scss optimisation #3050
Header scss optimisation #3050
Conversation
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
skjnldsv
added
3. to review
skjnldsv
requested review from
rullzer,
MorrisJobke,
jancborchardt and
ChristophWurst
| @@ -1,3 +1,10 @@ | |||
| /** | |||
| * @copyright Copyright (c) 2017, John Molakvoæ (skjnldsv@protonmail.com) | |||
There was a problem hiding this comment.
We also need to include the original authors of the original file ;)
There was a problem hiding this comment.
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.
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.
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.
Same here, separate lines per selector for readability please. :)
| @@ -39,7 +45,6 @@ | |||
| } | |||
There was a problem hiding this comment.
Also if you want to do this in the right way, please don't use "magical" values you should assign variables like this to z-index.
There was a problem hiding this comment.
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.
In here instead of repeating the code you why you don't use @extend?
There was a problem hiding this comment.
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.
@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.
I created a script to output the original copyrights from the commits.
I will create a special pr to put all the copyrights correctly :)
skjnldsv
dismissed
jancborchardt’s stale review
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! 😭 |
|
Sorry @ChristophWurst 😉 |
Optimisation and simplification of the header.scss file
@nextcloud/designers