-
-
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
Switches the default logo color depending on the primary color #10898
Switches the default logo color depending on the primary color #10898
Conversation
core/img/logo-blue.svg
Outdated
@@ -0,0 +1,76 @@ | |||
<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
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 added the blue versions and didn't use the svg api, because it may not be available e.g. on the guest page.
There was another issue also taking out some icons for that reason.
@weeman1337 cool! I will come over to you in a bit :) Meanwhile – what would happen, say, when you have a light yellow or green theming color? Cause then the logo color would clash, right? That's why I would make it dark instead of blue, just like the top bar icons. |
@jancborchardt I made it blue because of #10684 (comment)
|
@skjnldsv I moved the logos to |
So @weeman1337 and me just looked at it and we just have to pick a tradeoff:
So we are going for option 3. cc for info @nextcloud/designers Only thing to adjust is that the threshold for change is the same as for the app icons in the top bar. :) Then 👍 from me. |
Please wait for the stable14 branch until merging. I would like to not merge this into 14 that short before a release. |
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.
Good stuff :)
Some small nitpicks ;)
apps/theming/css/theming.scss
Outdated
|
||
@if ($has-custom-logo == false) { | ||
@if ($invert) { | ||
$image-logo: url('../../../../svg/core/logo/logo/000000?v=1'); |
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.
Please use the scss function for that :)
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.
server/core/css/functions.scss
Lines 34 to 48 in 7210b11
@mixin icon-color($icon, $dir, $color, $version: 1, $core: false) { | |
// remove # from color | |
// inspect cast int to string | |
$index: str-index(inspect($color), '#'); | |
@if $index { | |
$color: str-slice(inspect($color), 2); | |
} | |
$varName: "--icon-#{$icon}-#{$color}"; | |
@if $core { | |
#{$varName}: url('#{$webroot}/svg/core/#{$dir}/#{$icon}/#{$color}?v=#{$version}'); | |
} @else { | |
#{$varName}: url('#{$webroot}/svg/#{$dir}/#{$icon}/#{$color}?v=#{$version}'); | |
} | |
background-image: var(#{$varName}); | |
} |
core/img/logo/logo.svg
Outdated
@@ -0,0 +1,71 @@ | |||
<?xml version="1.0" encoding="UTF-8" standalone="no"?> |
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.
why this change of the logo 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 just removed the grouping of the three circles. The svg coloring freaked out..
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.
Can you clean things up then? :)
We use scour ;)
scour --create-groups --enable-id-stripping --enable-comment-stripping --shorten-ids --remove-metadata --strip-xml-prolog --no-line-breaks -i old.svg new.svg
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.
done
core/Controller/SvgController.php
Outdated
@@ -119,8 +119,7 @@ private function getSvg(string $path, string $color, string $fileName) { | |||
} | |||
|
|||
// add fill (fill is not present on black elements) | |||
$fillRe = '/<((circle|rect|path)((?!fill)[a-z0-9 =".\-#():;])+)\/>/mi'; | |||
|
|||
$fillRe = '/<((circle|rect|path)((!fill)[a-z0-9 =".\-#():;])+)\/>/mi'; |
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.
If you change the regex, please update the tests to make sure we also cover the reason why you changed this ;)
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'll add a test!
It changes the regex to do what the comment says ;) The actual version replaces existing fills. Removing the ? only matches if there isn't a fill already.
7210b11
to
0576f82
Compare
@skjnldsv I added a SvgControllerTest. The SVG coloring has some pitfalls. E.g. applying it on a black circle (without stroke/fill) or a group (applying it to child elements) may result in unwanted filled elements. I tweaked the Nextcloud logo to work. With the new test we may extend the behavior in the future. |
* @param string $icon the icon filename | ||
* @param string $dir the icon folder within /core/img if $core or app name | ||
* @param string $color the desired color in hexadecimal | ||
* @param int $version the version of the file | ||
* @param bool [$core] search icon in core | ||
* | ||
* @returns string the url to the svg api endpoint | ||
* @returns A background image with the url to the set to the requested icon. | ||
*/ | ||
@mixin icon-color($icon, $dir, $color, $version: 1, $core: false) { |
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 took this apart, because it didn't do what the comment said...
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.
Thx!
Maybe we should enforce a fill attribute and only replace in the future :) |
Could this be implemented for the favicon as well? Would be nice to have a favicon that corresponds to the primary color and the different color of the Nextcloud logo. Currently it is necessary to use imagmagick to do any changes to the favicon. |
@suntorytimed unfortunately, no. Svg as favicon are nowhere near to be supported: https://caniuse.com/#feat=link-icon-svg |
0576f82
to
d1478a9
Compare
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
d1478a9
to
a45ec3d
Compare
Rebased |
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 looks good! Let's wait the input of our lord and master @juliushaertl 😉
@weeman1337 There is one test failing:
Besides that it looks good to me, the favicon could also use the inverted logo, as we generate a png from with imagemagick for that anyway, but we can do that in a follow up pr. |
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.
Nice, looking good! 👍
And yes, @juliushaertl follow-up PR to adjust the favicon would be great! :)
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
@weeman1337 Tests still fail 😢 |
Signed-off-by: Michael Weimann <mail@michael-weimann.eu>
These changes add switching the default logo from white to blue depending on the primary color.
The switch happens between #cccccc and #dddddd
A custom logo doesn't use this mechanism.
Closes #10684