React to aspnet/Razor#221: Modify TagHelpers to utilize new ContentMode design. #1795
Conversation
|
/cc @pranavkm |
NTaylorMullen
force-pushed
the
razor.thcontentmode.221
branch
from
3b2df62 to
6735c30
Compare
| @@ -13,7 +13,7 @@ | |||
| <form asp-anti-forgery="false" asp-action="Create"> | |||
| <div class="form-horizontal"> | |||
| @* validation summary tag helper will target just <div/> elements and append the list of errors *@ | |||
| @* - i.e. this helper, like <select/> helper, has ContentBehavior.Append *@ | |||
| @* - i.e. this helper, like <select/> helper appends content via *@ | |||
|
⌚ |
|
Updated. |
| { | ||
| output.Content = tagBuilder.InnerHtml; | ||
| output.Content = childContent; |
There was a problem hiding this comment.
nit: this is hard on the eyes due to repeated conditions. suggest instead
if (string.IsNullOrEmpty(output.Content))
{
if (string.IsNullOrWhiteSpace(childContent))
{
// Provide default label text since there was nothing useful in the Razor source.
output.Content = tagBuilder.InnerHtml;
}
else
{
output.Content = childContent;
}
}
|
⌚ for reverting tagHelperContext -> context churn. (that'll make it easier to see real changes.) otherwise looks very close. |
|
Updated. |
| // We check for whitespace to detect scenarios such as: | ||
| // <label for="Name"> | ||
| // </label> | ||
| if (string.IsNullOrWhiteSpace(output.Content)) | ||
| if (string.IsNullOrEmpty(output.Content)) |
There was a problem hiding this comment.
late thought: should this check output.ContentSet instead?
There was a problem hiding this comment.
Definitely up for discussion. However, i'm leaning towards leaving it as is purely for the ability for TagHelpers to reset the output.Content if they please (reset = setting to string.Empty or null). If you're only ever checking ContentSet then once it's set there's no going back.
There was a problem hiding this comment.
let's rope in @DamianEdwards tomorrow. these tag helpers generally do not override what previous ones in the pipeline have done. diverging from that here.
|
⌚ for just a few open questions |
|
Updated. |
NTaylorMullen
force-pushed
the
razor.thcontentmode.221
branch
from
8266a19 to
da6387f
Compare
|
Rebased, verified test pass and updated. |
| [InlineData("Content of validation message", "Some Content", "Some Content")] | ||
| [InlineData("\r\n \r\n", "\r\n Something Else \r\n", "\r\n Something Else \r\n")] | ||
| [InlineData("\r\n \r\n", "Some Content", "Some Content")] | ||
| public async Task ProcessAsync_DoesntOverrideOutputContent( |
|
|
with one small fix and a rebase (since you signed off on - React to aspnet/Razor#221 - Modified existing TagHelpers to no longer rely on ContentBehavior and to instead utilize GetChildContentAsync, PreContent, Content and PostContent.
- React to aspnet/Razor#221 - Modified existing TagHelper tests to no longer rely on ContentBehavior. - Updated signatures of TagHelperExecutionContext and TagHelperContext pieces.
NTaylorMullen
force-pushed
the
razor.thcontentmode.221
branch
from
da6387f to
eb1eca9
Compare
Modify TagHelpers to use new content mode design.
Modify TagHelper tests to abide by new content mode design.