Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support both Text & HTML in the same email #14715
base: main
Are you sure you want to change the base?
Support both Text & HTML in the same email #14715
Changes from 13 commits
d492809
fefd6ff
57403f1
9d56318
9f52deb
5843a39
a587476
dd37fd1
a0bc7a6
3c079dd
4f8b04f
2d953d7
b8beb21
0ea8cd1
c3a546d
5952079
c86399f
d1f6ce7
279ae07
55aa5d6
024a39b
e964ff9
0fbca09
c065933
1bdc389
8127588
cc4604e
e11b9ab
daaf9dc
d228031
e817142
24eb630
e250ee4
3a5bca5
fa5d5ba
5e48b15
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I get this when opening a previously existing Email Task:
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.
This doesn't work for me either. Shouldn't this be like
on('change,...)
?Additionally, I think the
select
element needs its own id.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.
Since
GetProperty()
below uses this as the name by default, this will break existing Email Tasks, since they'll be empty. Add a migration that reads out the bodies of all Email Tasks, and depending onIsHtmlBody
, updates one of the fields with the old body value.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.
The idea is to generate both bodies, so
isHtmlBody
is not make sense hereThere 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.
For the consumers of the old API, nothing will change. For the consumers of the new API,
Obsolete
will tell that they shouldn't use this.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.
Have you checked that actually both are sent if both are provided?
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.
With this logic, if you switch from Text to HTML (not All), for example, then the old Text value will still exist, and will be used, even though you won't see it and won't be able to edit it on the UI.
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.
Since this is a published interface, keep the old properties with
Obsolete
, and make them use the newBody
behind the scenes.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.
Add convenience methods for
HasText()
andHasHtml()
that usestring.IsNullOrEmpty()
.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.
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.
I used
Text
something similar toBodyBuilder.Text
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.
I just think that in contrast to "HTML" it's clearer to have "plain text". It's also a term commonly used for this when talking about e-mail.
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.
No problem we can rename it, but I usually "PlainText" in security :)
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.
Maybe
IHtmlContent
instead to be better typed? With a convenience conversion method to/from string.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.
For simplicity, I prefer
string
instead, like other email messaging APIsThere 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.
Yeah but
string
is really overused.IHtmlContent
is for when you want to store a string that's actually HTML.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.
I don't think using
IHtmlContent
here is appropriate. That interface is for writing asp.net output, not mail messages.