Separate Email Logo #26871
Separate Email Logo #26871
Conversation
Signed-off-by: Johannes Brückner <johannes@dotplex.com>
|
Hi! Did anyone get a chance to take a look at it yet? |
ChristophWurst
requested review from
skjnldsv,
julien-nc,
artonge and
ArtificialOwl
and removed request for
ChristophWurst
|
I noticed that I didn't properly link to the issues I mentioned above. I fixed all of them and would still love to get some feedback. 🙄 |
apps/theming/lib/ThemingDefaults.php
Outdated
| 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.
A bit more comments on the code please? :)
Maybe something like
Fallback to logo if there is no emailLogo registered
There was a problem hiding this comment.
The new refactor based on @juliushaertl suggestion to extract it into a new getEmailLogo function greatly simplified the code, aligning it with the original getLogo function, so I think no additional comments are needed there. You're right though, I should've added some comments in this case. 👌
There was a problem hiding this comment.
I'd generally be ok with adding a setting for this, but I'd pull in @jancborchardt here for some additional checking. 😉
apps/theming/lib/ThemingDefaults.php
Outdated
| @@ -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.
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 OCP\Defaults like getMailLogo.
There was a problem hiding this comment.
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 getEmailLogo doesn't have to be concerned with SVGs now. I'm just not sure how to restrict the email logo upload field to certain mime-types... 🤔 Any idea?
| @@ -86,6 +86,7 @@ public function getImageUrl(string $key, bool $useSvg = true): string { | |||
|
|
|||
| switch ($key) { | |||
| case 'logo': | |||
| case 'emailLogo': | |||
There was a problem hiding this comment.
Since svg is causing issues with email it would make sense to not allow to upload it then for an email logo:
There was a problem hiding this comment.
Yep, you're absolutely right! The refactored version (see my other comment below) has no support for an SVG version.
@brueckner @juliushaertl is it not possible to fix the SVG-to-PNG generation? As usual having a setting means that every single person has to set this, whereas if we would fix the generator, that'd be a whole lot of work saved for our end users. |
It's not (only) about the blurry appearance of the logo, as some users have described in other issues. It's much more about that fact that you might want to have a completly different logo for your emails that suits the styling of the mails sent out by Nextcloud. The idea is to use the regular logo by default, so people don't have to use the setting at all, but this is part of a theming app that is supposed to offer customization, so offering some additional customization options won't hurt anyone I suppose. ;) Sidenote: the current (refactored) verison does not fall back to the regular site logo (yet). I wanted to wait for some feedback on it before going the extra mile. |
|
@jancborchardt so how to proceed with this then? |
|
/rebase |
|
Having different logos might also be interesting here in cases where the small rendering of the email logo does not work well with the one that is supposed at the login page. The header logo can already be set separately but the one on used the login page might be considered for larger use and have detail that don't work well on the small size of the mail. |
|
What's the status here? |
|
Needs some decision if we want this in with an extra setting. |
but it seems like there isn't a solution for email logos using an SVG custom logo? the SVG conversion is broken especially in snap (I respect the safety concerns regarding imagick) |
This comment has been minimized.
This comment has been minimized.
skjnldsv
added
2. developing
|
What is the current Status? |
ChristophWurst
added
0. Needs triage
skjnldsv
added
2. developing
|
As this sounds like a nice feature, the requests for this are quite low. Currently there a no plans to implement such a feature. Thus I will close this ticket for now. Thanks for the interest in Nextcloud and the effort put into this! 🙇 |
This PR adds the ability to upload a separate logo for notification emails.
I basically copied most of the funtionality from the 'logo' upload field and added a fallback check in
apps/theming/lib/ThemingDefaults.php. If no email logo is provided, the default logo will be used and if that's not present either, the default nextcloud logo will be used.Related Issues:
Kind of related: