Skip to content
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

Fix primary color usage in emails and federation #36348

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Jan 25, 2023

  • 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 1 From person 2
Bildschirmfoto vom 2023-01-24 17-08-23 Bildschirmfoto vom 2023-01-24 17-08-29

Screenshots

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

Checklist

@nickvergessen nickvergessen added this to the Nextcloud 26 milestone Jan 25, 2023
@nickvergessen nickvergessen requested review from skjnldsv and a team January 25, 2023 10:00
@nickvergessen nickvergessen self-assigned this Jan 25, 2023
@nickvergessen nickvergessen requested review from Pytal and szaimen and removed request for a team January 25, 2023 10:00
@nickvergessen nickvergessen changed the title Bugfix/noid/fix primary color usage in emails and federation Dix primary color usage in emails and federation Jan 25, 2023
@nickvergessen nickvergessen changed the title Dix primary color usage in emails and federation Fix primary color usage in emails and federation Jan 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
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
Member Author

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.

public function getColorPrimary() {
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
Member

Choose a reason for hiding this comment

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

ok then

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

🐘

@nickvergessen
Copy link
Member Author

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 nickvergessen force-pushed the bugfix/noid/fix-primary-color-usage-in-emails-and-federation branch from 6335d76 to 959e2aa Compare January 30, 2023 08:13
@nickvergessen nickvergessen merged commit de415fb into master Jan 30, 2023
@nickvergessen nickvergessen deleted the bugfix/noid/fix-primary-color-usage-in-emails-and-federation branch January 30, 2023 08:56
@blizzz blizzz mentioned this pull request Feb 1, 2023
Sign up for free to 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