Skip to content

Fix primary color usage in emails and federation - #36348

Merged
nickvergessen merged 3 commits into
masterfrom
bugfix/noid/fix-primary-color-usage-in-emails-and-federation
Jan 30, 2023
Merged

Fix primary color usage in emails and federation#36348
nickvergessen merged 3 commits into
masterfrom
bugfix/noid/fix-primary-color-usage-in-emails-and-federation

Conversation

@nickvergessen

@nickvergessennickvergessen commented Jan 25, 2023

Copy link
Copy Markdown
Member
  • Resolves: N/A

Summary

  • Federation box to be implemented on own website also used the background image color instead of the instance theme
  • Emails used the color of the triggering user, so if you invite 5 users with different custom background image to an event, you will get 5 differently colored emails instead.
    2 sample emails from the same day, same instance, no settings changed on my side:
From person 1From person 2
Bildschirmfoto vom 2023-01-24 17-08-23Bildschirmfoto vom 2023-01-24 17-08-29

Screenshots

  • Theming color: #DF2254
  • User custom picked theming color: #ffffff
BeforeAfter
Bildschirmfoto vom 2023-01-25 10-46-02Bildschirmfoto vom 2023-01-25 10-44-19
Bildschirmfoto vom 2023-01-25 10-52-59Bildschirmfoto vom 2023-01-25 10-39-40

Checklist

@nickvergessennickvergessen added this to the Nextcloud 26 milestone Jan 25, 2023
@nickvergessen
nickvergessen requested review from a team and skjnldsvJanuary 25, 2023 10:00
@nickvergessennickvergessen self-assigned this Jan 25, 2023
@nickvergessen
nickvergessen requested review from Pytal and szaimen and removed request for a teamJanuary 25, 2023 10:00
@nickvergessennickvergessen changed the title Bugfix/noid/fix primary color usage in emails and federationDix primary color usage in emails and federationJan 25, 2023
@nickvergessennickvergessen changed the title Dix primary color usage in emails and federationFix primary color usage in emails and federationJan 25, 2023
Comment on lines +213 to +218
public function getDefaultColorPrimary(): string {
if (method_exists($this->defaults, 'getDefaultColorPrimary')) {
return $this->defaults->getDefaultColorPrimary();
}
return $this->defaults->getColorPrimary();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getDefaultColorPrimary should always be available no?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OC_Defaults only has getColorPrimary and I'm unwilling to debug which version of the defaults object is used where and why.

publicfunctiongetColorPrimary() {
if ($this->themeExist('getColorPrimary')) {
return$this->theme->getColorPrimary();
}
if ($this->themeExist('getMailHeaderColor')) {
return$this->theme->getMailHeaderColor();
}
return$this->defaultColorPrimary;
}

That's a cleanup task for another day.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok then

@szaimenszaimen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@nickvergessen

Copy link
Copy Markdown
MemberAuthor

Of course there are tests which checks which method is invoked, when getting color in the email... will have a look tomorrow.

Signed-off-by: Joas Schilling <coding@schilljs.com>
…user triggering the email
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen
nickvergessenforce-pushed the bugfix/noid/fix-primary-color-usage-in-emails-and-federation branch from 6335d76 to 959e2aaCompareJanuary 30, 2023 08:13
@nickvergessen
nickvergessen merged commit de415fb into masterJan 30, 2023
@nickvergessen
nickvergessen deleted the bugfix/noid/fix-primary-color-usage-in-emails-and-federation branch January 30, 2023 08:56
@blizzzblizzz mentioned this pull request Feb 1, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@nickvergessen@skjnldsv@Pytal@szaimen