Add ITagHelperComponent #1096
Add ITagHelperComponent #1096
Conversation
| public interface ITagHelperComponent | ||
| { | ||
| /// <summary> | ||
| /// When a set of<see cref= "ITagHelperComponent" />s are executed, their <see cref="Init(TagHelperContext)"/>'s |
|
|
||
| namespace Microsoft.AspNetCore.Razor.Runtime.TagHelpers | ||
| { | ||
| public class TagHelperComponent : ITagHelperComponent |
| public virtual void Init(TagHelperContext context) | ||
| { | ||
| } | ||
|
|
There was a problem hiding this comment.
Probably makes sense to add a helper Process method here like we have on TagHelper
| @@ -14,7 +14,7 @@ namespace Microsoft.AspNetCore.Razor.Runtime.TagHelpers | |||
| public class TagHelperRunner | |||
| { | |||
| /// <summary> | |||
| /// Calls the <see cref="ITagHelper.ProcessAsync"/> method on <see cref="ITagHelper"/>s. | |||
| /// Calls the <see cref="ITagHelperComponent.ProcessAsync"/> method on <see cref="ITagHelper"/>s. | |||
There was a problem hiding this comment.
This should still say ITagHelper
There was a problem hiding this comment.
@jbagga do we need to change this still?
There was a problem hiding this comment.
It cannot resolve ITagHelper.ProcessAsync since the definition is now in ITagHelperComponent
There was a problem hiding this comment.
Ah, I see. Then I think it's fine as-is.
| /// Returns <c>true</c> to indicate if <see cref="ITagHelperComponent"/> applies to the given <see cref="TagHelperContext"/>. | ||
| /// </summary> | ||
| /// <param name="context">Contains information associated with the current HTML tag.</param> | ||
| public virtual bool AppliesTo(TagHelperContext context) |
There was a problem hiding this comment.
Who calls AppliesTo? If it's going to be on the convenience-base-class it needs to be called.
There was a problem hiding this comment.
Really good point. The bits here in Razor are mostly to facilitate a TagHelper looking type that isn't actually a TagHelper. @DamianEdwards did you envision the AppliesTo functionality living in Razor or letting ITagHelperComponent authors do their own IF statements?
There was a problem hiding this comment.
Talked with @DamianEdwards offline. Lets remove this method and allow people who want to write TagHelperComponent's write their own if statements 😄. Note that this also removes the need to log "taghelper component applies to X"
| namespace Microsoft.AspNetCore.Razor.TagHelpers | ||
| { | ||
| /// <summary> | ||
| /// Contract used to filter matching HTML elements. |
There was a problem hiding this comment.
The doc comment here is identical to the one for ITagHelper (minus the "marker" comment). How can we do a better job distinguishing the docs for these two different types?
There was a problem hiding this comment.
@NTaylorMullen Should we explain the difference between ITagHelper and ITagHelperComponent in the doc comments?
There was a problem hiding this comment.
I wouldn't explain the difference between the two. I'd explain that ITagHelperComponent enables users to mutate the representation of an HTML element. ITagHelper on the other hand is still a contract to filter matching HTML elements.
There was a problem hiding this comment.
Yeah I think each should just say what they are and what they're for.
There was a problem hiding this comment.
@NTaylorMullen Why should I nuke the doc comment for ITagHelper then? #1096 (comment)
There was a problem hiding this comment.
@NTaylorMullen Why should I nuke the doc comment for ITagHelper then? #1096 (comment)
It was an old comment prior to me noticing the doc comment similarities. Deleted the comment 😄
jbagga
force-pushed
the
jbagga/TagHelperComponentMvc5728
branch
from
1b943a1 to
9167c16
Compare
|
@NTaylorMullen @Eilon Updated. Please review the docs. I am not very happy with the base class docs |
| namespace Microsoft.AspNetCore.Razor.TagHelpers | ||
| { | ||
| /// <summary> | ||
| /// Contract used to enable the modification of an HTML element's representation. |
There was a problem hiding this comment.
Contract used to modify an HTML element.
| public interface ITagHelperComponent | ||
| { | ||
| /// <summary> | ||
| /// When a set of <see cref= "ITagHelperComponent" />s are executed, their <see cref="Init(TagHelperContext)"/>'s |
| /// </summary> | ||
| public abstract class TagHelper : ITagHelper | ||
| { | ||
| /// <inheritdoc /> | ||
| /// <summary> | ||
| /// When a set of <see cref= "ITagHelper" />s are executed, their <see cref="Init(TagHelperContext)"/>'s |
jbagga
force-pushed
the
jbagga/TagHelperComponentMvc5728
branch
from
9167c16 to
a2fb67c
Compare
|
@NTaylorMullen @Eilon Updated |
jbagga
force-pushed
the
jbagga/TagHelperComponentMvc5728
branch
from
a6461fa to
3637a25
Compare
Fix build In reaction to aspnet/Razor#1096
Refactor PR: aspnet/Mvc#5966
Issue: aspnet/Mvc#5728
cc @NTaylorMullen @rynowak