-
-
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
Separate Email Logo #26871
Separate Email Logo #26871
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -223,8 +223,10 @@ public function getColorPrimary() { | |
* @param bool $useSvg Whether to point to the SVG image or a fallback | ||
* @return string | ||
*/ | ||
public function getLogo($useSvg = true): string { | ||
$logo = $this->config->getAppValue('theming', 'logoMime', ''); | ||
public function getLogo($useSvg = true, $emailLogo = false): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm a bit concerned with just adding another argument here so since this is a totally different logo file that can be uploaded I'd rather go with a separate method also in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That a very good point, thanks for bringing it up! I was actually thinking about that at first, but was afraid to get complains with regards to code duplication. However, having done the refactor now it feels much, much better and is totally worth a little duplication imho. Especially with regards to your other comment about the SVG - the new |
||
$logoKey = $emailLogo ? 'emailLogo' : 'logo'; | ||
$logo = $this->config->getAppValue('theming', $logoKey . 'Mime', ''); | ||
$cacheBusterCounter = $this->config->getAppValue('theming', 'cachebuster', '0'); | ||
|
||
// short cut to avoid setting up the filesystem just to check if the logo is there | ||
// | ||
|
@@ -235,14 +237,23 @@ public function getLogo($useSvg = true): string { | |
$logoExists = true; | ||
} else { | ||
try { | ||
$this->imageManager->getImage('logo', $useSvg); | ||
$this->imageManager->getImage($logoKey, $useSvg); | ||
$logoExists = true; | ||
} catch (\Exception $e) { | ||
$logoExists = false; | ||
} | ||
} | ||
|
||
$cacheBusterCounter = $this->config->getAppValue('theming', 'cachebuster', '0'); | ||
if (!$logoExists) { | ||
try { | ||
$logoKey = 'logo'; | ||
$logo = $this->config->getAppValue('theming', $logoKey . 'Mime', ''); | ||
$this->imageManager->getImage($logoKey, $useSvg); | ||
$logoExists = true; | ||
} catch (\Exception $e) { | ||
$logoExists = false; | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A bit more comments on the code please? :)
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new refactor based on @juliushaertl suggestion to extract it into a new |
||
} | ||
|
||
if (!$logo || !$logoExists) { | ||
if ($useSvg) { | ||
|
@@ -253,7 +264,7 @@ public function getLogo($useSvg = true): string { | |
return $logo . '?v=' . $cacheBusterCounter; | ||
} | ||
|
||
return $this->urlGenerator->linkToRoute('theming.Theming.getImage', [ 'key' => 'logo', 'useSvg' => $useSvg, 'v' => $cacheBusterCounter ]); | ||
return $this->urlGenerator->linkToRoute('theming.Theming.getImage', [ 'key' => $logoKey, 'useSvg' => $useSvg, 'v' => $cacheBusterCounter ]); | ||
} | ||
|
||
/** | ||
|
@@ -300,12 +311,14 @@ public function getScssVariables() { | |
$variables = [ | ||
'theming-cachebuster' => "'" . $cacheBuster . "'", | ||
'theming-logo-mime' => "'" . $this->config->getAppValue('theming', 'logoMime') . "'", | ||
'theming-email-logo-mime' => "'" . $this->config->getAppValue('theming', 'emailLogoMime') . "'", | ||
'theming-background-mime' => "'" . $this->config->getAppValue('theming', 'backgroundMime') . "'", | ||
'theming-logoheader-mime' => "'" . $this->config->getAppValue('theming', 'logoheaderMime') . "'", | ||
'theming-favicon-mime' => "'" . $this->config->getAppValue('theming', 'faviconMime') . "'" | ||
]; | ||
|
||
$variables['image-logo'] = "url('".$this->imageManager->getImageUrl('logo')."')"; | ||
$variables['image-email-logo'] = "url('".$this->imageManager->getImageUrl('emailLogo')."')"; | ||
$variables['image-logoheader'] = "url('".$this->imageManager->getImageUrl('logoheader')."')"; | ||
$variables['image-favicon'] = "url('".$this->imageManager->getImageUrl('favicon')."')"; | ||
$variables['image-login-background'] = "url('".$this->imageManager->getImageUrl('background')."')"; | ||
|
@@ -424,6 +437,7 @@ public function undo($setting) { | |
$returnValue = $this->getColorPrimary(); | ||
break; | ||
case 'logo': | ||
case 'emailLogo': | ||
case 'logoheader': | ||
case 'background': | ||
case 'favicon': | ||
|
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.
Since svg is causing issues with email it would make sense to not allow to upload it then for an email logo:
https://github.com/nextcloud/server/pull/26871/files#diff-b9d526f6249d406c08009d9de5802b5637194bb66ec759df798dd397f9a422beR222-R225
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.
Yep, you're absolutely right! The refactored version (see my other comment below) has no support for an SVG version.