-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Add lists to HTML emails #4467
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
Add lists to HTML emails #4467
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
|
@nickvergessen, thanks for your PR! By analyzing the history of the files in this pull request, we identified @MorrisJobke, @LukasReschke and @schiessle to be potential reviewers. |
| * @param string $plainText Text that is used in the plain text email | ||
| * if empty the $text is used | ||
| * @since 12.0.0 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a bit more sophisticated? Like "Main text" and then meta text (for the date for example) and an optional icon? (for small file previews)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it's a simple list, not one trimmed down so it only works for activity emails :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? I think list can have this optional values or not (meta is shown in small above the main text if present) and the icon is also optionally shown if present. This also could be a list of users with avatars or something like that. If both are not supplied it's a plain list - with it, it's a bit more sophisticated and nice looking.
Signed-off-by: Joas Schilling <coding@schilljs.com>
Codecov Report
@@ Coverage Diff @@
## master #4467 +/- ##
============================================
+ Coverage 54.11% 54.11% +<.01%
- Complexity 21679 21684 +5
============================================
Files 1331 1331
Lines 83066 83087 +21
Branches 1312 1312
============================================
+ Hits 44952 44964 +12
- Misses 38114 38123 +9
|
MorrisJobke
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good and makes sense - also works with the activity email 👍
rullzer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Required for nextcloud/activity#141