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.

Sign up for GitHub

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

Jump to bottom

Fix design and layout of notification mails #20380

Merged
merged 6 commits into from
May 1, 2020
Merged

Fix design and layout of notification mails #20380

merged 6 commits into from
May 1, 2020

Conversation

jancborchardt
Copy link
Member

@jancborchardt jancborchardt commented Apr 9, 2020 •
edited

Also is a step towards simplifying the email template as per #9119

Just a question: How best to trigger the mails for testing? So far I did it inline in the browser.

Before

Notification mail old top

When reading notifications mails with the Mail app, you have to scroll down to see any content:
Notification mail old bottom

New

Notification mail new

@jancborchardt jancborchardt added 3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: emails labels Apr 9, 2020
@jancborchardt jancborchardt added this to the Nextcloud 19 milestone Apr 9, 2020
@jancborchardt jancborchardt mentioned this pull request Apr 9, 2020
Closed
@szaimen
Copy link
Contributor

szaimen commented Apr 9, 2020 •
edited

@jancborchardt I think it looks much better then before but I would center-align the activity notification if there is only one. It would look then something like this:
image

@rullzer rullzer mentioned this pull request Apr 9, 2020
Merged
59 tasks
@jancborchardt
Copy link
Member Author

I would center-align the activity notification if there is only one. It would look then something like this:

Sure – but let’s do it in a follow-up, already touched this beast enough for now. :) There’s plenty of other stuff to fix as well still.

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 9, 2020
@ChristophWurst
Copy link
Member

Test\Mail\EMailTemplateTest fails

@ChristophWurst ChristophWurst added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Apr 9, 2020
@jancborchardt
Copy link
Member Author

jancborchardt commented Apr 12, 2020 •
edited

Test\Mail\EMailTemplateTest fails

So do I need to manually copy all the changes to:

  • tests/data/emails/new-account-email.html
  • tests/data/emails/new-account-email-custom.html
  • tests/data/emails/new-account-email-single-button.html
  • apps/settings/tests/Mailer/NewUserMailHelperTest.php

Or is there a simpler way?

@jancborchardt jancborchardt mentioned this pull request Apr 14, 2020
Merged
@kesselb
Copy link
Contributor

kesselb commented Apr 14, 2020

Logo needs a lot of space now. For me it's to much present. That email should be about the activity and not the instance logo.

@jancborchardt
Copy link
Member Author

Logo needs a lot of space now. For me it's to much present. That email should be about the activity and not the instance logo.

I can reduce the padding a bit. Do you know something regarding my question on fixing the tests? :)

@kesselb
Copy link
Contributor

kesselb commented Apr 14, 2020

master
Screenshot from 2020-04-14 17-03-36

this pr
Screenshot from 2020-04-14 17-03-21

The email settings test mail looks a bit broken.

@kesselb
Copy link
Contributor

kesselb commented Apr 14, 2020

Do you know something regarding my question on fixing the tests? :)

The tests compare the generated html with the one saved at tests/data/emails/. You may update the expected output there.

@rullzer rullzer mentioned this pull request Apr 15, 2020
Merged
57 tasks
@jancborchardt
Copy link
Member Author

Ok, so as asked I need to just manually copy it over into these files?

  • tests/data/emails/new-account-email.html
  • tests/data/emails/new-account-email-custom.html
  • tests/data/emails/new-account-email-single-button.html
  • apps/settings/tests/Mailer/NewUserMailHelperTest.php

Or is there a simpler way?

As for the layout bug, I will remove that box-shadow – thanks for catching it!

@rullzer rullzer mentioned this pull request Apr 16, 2020
Merged
55 tasks
@georgehrke
Copy link
Member

As for the layout bug, I will remove that box-shadow – thanks for catching it!

Bump

@georgehrke georgehrke self-requested a review April 24, 2020 14:46
@rullzer rullzer modified the milestones: Nextcloud 19, Nextcloud 20 Apr 30, 2020
@jancborchardt
Fix notification mails height issue in Nextcloud Mail
1d5e7b3 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Notification mails: Adjust vertical spacing
43371c9 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Notification mails: Logo bar/box to circle so it stands nicely on its…
b38c3bf 
… own

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Notification mails: Change font to native font stack
fbef270 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Notification mails: Fix some alignment details and remove shadow
d582315 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt jancborchardt added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Apr 30, 2020
@jancborchardt
Copy link
Member Author

Rebased, removed the box-shadow, and fixed the tests. @georgehrke @kesselb @ChristophWurst

@jancborchardt jancborchardt force-pushed the design/notification-mails branch 2 times, most recently from 19faf4a to d679b4b Compare April 30, 2020 18:04
@rullzer rullzer mentioned this pull request Apr 30, 2020
Merged
2 tasks
@jancborchardt
Copy link
Member Author

The test sudo NOCOVERAGE=1 ./autotest.sh sqlite tests/lib/Mail/EMailTemplateTest.php passes locally, so I don’t understand why https://drone.nextcloud.com/nextcloud/server/28545/10/4 shows errors?

@kesselb
Copy link
Contributor

kesselb commented Apr 30, 2020

@jancborchardt OCA\Settings\Tests\Mailer\NewUserMailHelperTest is the failing test. There is also some html markup to adjust.

@jancborchardt
Copy link
Member Author

@jancborchardt OCA\Settings\Tests\Mailer\NewUserMailHelperTest is the failing test. There is also some html markup to adjust.

How can I run this test locally? Sorry for the stupid questions, but the documentation was not very clear or gave me commands that didn’t work, and it’s not very obvious how to run the tests. (Also I’m a designer, not dev. ;)

@kesselb
Copy link
Contributor

kesselb commented Apr 30, 2020

Good question. I think the same command as for the other test should work.

The test is located here: apps/settings/tests/Mailer/NewUserMailHelperTest.php

@gary-kim
Copy link
Member

Yeah, NOCOVERAGE=1 ./autotest.sh sqlite apps/settings/tests/Mailer/NewUserMailHelperTest.php will work.

@jancborchardt
Copy link
Member Author

The test is located here: apps/settings/tests/Mailer/NewUserMailHelperTest.php

Aaah ok, didn’t know it was somewhere else. :) Thank you both @kesselb @gary-kim! :)

@jancborchardt
Fix notification mail tests
e981669 
Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt
Copy link
Member Author

Aaand all tests fixed now! :) That is, both of these:

  • NOCOVERAGE=1 ./autotest.sh sqlite apps/settings/tests/Mailer/NewUserMailHelperTest.php
  • NOCOVERAGE=1 ./autotest.sh sqlite tests/lib/Mail/EMailTemplateTest.php

@rullzer rullzer merged commit c724eb2 into master May 1, 2020
@rullzer rullzer deleted the design/notification-mails branch May 1, 2020 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@GretaD GretaD GretaD approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@juliushaertl juliushaertl Awaiting requested review from juliushaertl

@kesselb kesselb Awaiting requested review from kesselb

@georgehrke georgehrke Awaiting requested review from georgehrke

@gary-kim gary-kim Awaiting requested review from gary-kim

Assignees
No one assigned
Labels
3. to review Waiting for reviews bug design Design, UI, UX, etc. feature: emails
Projects
None yet
Milestone
Nextcloud 19
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@jancborchardt @szaimen @ChristophWurst @kesselb @simonspa @georgehrke @gary-kim @GretaD @rullzer