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

EMailTemplate: Fix button background in Lotus Notes #10291

Merged
merged 3 commits into from
Jul 19, 2018

Conversation

juliusknorr
Copy link
Member

I cannot test this, but according to #10052 and https://www.campaignmonitor.com/css/email-client/ibm-notes-9/ it should work. 🙈

@michag86 Can you maybe test this and prove that it works?

…endering in lotus notes

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr requested a review from rullzer July 19, 2018 05:53
@juliusknorr juliusknorr added bug design Design, UI, UX, etc. 3. to review Waiting for reviews labels Jul 19, 2018
@juliusknorr juliusknorr added this to the Nextcloud 14 milestone Jul 19, 2018
@michag86
Copy link
Contributor

Okay, I will test this.

@MorrisJobke
Copy link
Member

That also means that any possible changed templates out there will not work anymore, because of the changed order of parameters.

@MorrisJobke
Copy link
Member

@juliushaertl Could you fix the unit tests as you changed the result HTML

@juliusknorr
Copy link
Member Author

juliusknorr commented Jul 19, 2018

That also means that any possible changed templates out there will not work anymore, because of the changed order of parameters.

Ah so the custom templates only change the template strings :/ We should consider moving this to some template engine then, so we can have proper named parameters in the template string. (maybe something for 15)

@MorrisJobke
Copy link
Member

Ah so the custom templates only change the template strings :/ We should consider moving this to some template engine then, so we can have proper named parameters in the template string. (maybe something for 15)

Or just use named replacements ;) But yes - something for 15 and we document it just and are done ;)

@juliusknorr
Copy link
Member Author

Ok, let me fix the tests...

@@ -539,7 +539,7 @@ public function addBodyButtonGroup(string $textLeft,
$color = $this->themingDefaults->getColorPrimary();
$textColor = $this->themingDefaults->getTextColorPrimary();

$this->htmlBody .= vsprintf($this->buttonGroup, [$color, $color, $urlLeft, $color, $textColor, $textColor, $textLeft, $urlRight, $textRight]);
$this->htmlBody .= vsprintf($this->buttonGroup, [$color, $color, $color, $urlLeft, $color, $textColor, $textColor, $textLeft, $urlRight, $textRight]);
Copy link
Member

Choose a reason for hiding this comment

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

You can simply move the new value to the end and replace all %s with their %1$s equivalents.
That would keep compatibility

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/10052/fix-lotus-notes branch from 36b7f56 to 1213a7c Compare July 19, 2018 09:43
@juliusknorr
Copy link
Member Author

Thanks @nickvergessen for the hint 👍

Fixed that so we don't break compatibility. The failing test is unrelated.

@michag86
Copy link
Contributor

Changes from EMailTemplate.php are tested and working.

@MorrisJobke MorrisJobke merged commit 8032e36 into master Jul 19, 2018
@MorrisJobke MorrisJobke deleted the bugfix/10052/fix-lotus-notes branch July 19, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants