Add a <partial /> tag helper #7089

pranavkm · 2017-11-29T01:55:07Z

ajaybhargavb

LGTM

ajaybhargavb · 2017-11-29T02:53:36Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        private readonly IViewBufferScope _viewBufferScope;
+
+        public PartialTagHelper(
+            IHtmlGenerator htmlGenerator,


could just put it on one line.

ajaybhargavb · 2017-11-29T02:56:41Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        [ViewContext]
+        public ViewContext ViewContext { get; set; }
+
+        public override async Task ProcessAsync(TagHelperContext context, TagHelperOutput output)


null-check both parameters.

ajaybhargavb · 2017-11-29T03:06:32Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs

+                var newViewData = new ViewDataDictionary<object>(baseViewData, model);
+                var partialViewContext = new ViewContext(viewContext, view, newViewData, writer);
+
+                await viewEngineResult.View.RenderAsync(partialViewContext);


await view.RenderAsync(...)

ajaybhargavb · 2017-11-29T03:09:07Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/IHtmlGenerator.cs

+        /// <summary>
+        /// Renders HTML markup for the specified partial view.
+        /// </summary>
+        /// <param name="partialViewName">


Wrong order. Should be below ViewContext.

ajaybhargavb · 2017-11-29T03:11:15Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/IHtmlGenerator.cs

+        /// Renders HTML markup for the specified partial view.
+        /// </summary>
+        /// <param name="partialViewName">
+        /// The name of the partial view used to create the HTML markup. Must not be <c>null</c>.


Should probably remove Must not be null here or add it to both ViewContext and writer because we null check those.

ajaybhargavb · 2017-11-29T03:18:33Z

test/Microsoft.AspNetCore.Mvc.ViewFeatures.Test/ViewFeatures/DefaultHtmlGeneratorTest.cs

+                .Verifiable();
+            viewEngine
+                .Setup(v => v.FindView(It.IsAny<ActionContext>(), It.IsAny<string>(), /*isMainPage*/ false))
+                .Returns(ViewEngineResult.NotFound("test-view", new[] { "location3", "location4" }))


I assume we have a test somewhere else where GetView fails but FindView succeeds? If not, it might be worth adding one.

dougbu · 2017-11-29T05:44:38Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        }
+
+        /// <summary>
+        /// The name of the partial view used to create the HTML markup. Must not be <c>null</c>.


If this property must not be null, why doesn't the setter throw an ArgumentNullException?

dougbu · 2017-11-29T05:48:15Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        [ViewContext]
+        public ViewContext ViewContext { get; set; }
+
+        public override async Task ProcessAsync(TagHelperContext context, TagHelperOutput output)


null-check both parameters.

dougbu · 2017-11-29T05:54:51Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/breakingchanges.netcore.json

+  {
+    "TypeId": "public class Microsoft.AspNetCore.Mvc.ViewFeatures.DefaultHtmlGenerator : Microsoft.AspNetCore.Mvc.ViewFeatures.IHtmlGenerator",
+    "MemberId": "public .ctor(Microsoft.AspNetCore.Antiforgery.IAntiforgery antiforgery, Microsoft.Extensions.Options.IOptions<Microsoft.AspNetCore.Mvc.MvcViewOptions> optionsAccessor, Microsoft.AspNetCore.Mvc.ModelBinding.IModelMetadataProvider metadataProvider, Microsoft.AspNetCore.Mvc.Routing.IUrlHelperFactory urlHelperFactory, System.Text.Encodings.Web.HtmlEncoder htmlEncoder, Microsoft.AspNetCore.Mvc.ViewFeatures.ValidationHtmlAttributeProvider validationAttributeProvider)",
+    "Kind": "Removal"


Why make this breaking change? Can add a new constructor and deprecate the old.

Yeah no breaking changes please.

dougbu · 2017-11-29T05:55:34Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/breakingchanges.netcore.json

+  {
+    "TypeId": "public interface Microsoft.AspNetCore.Mvc.ViewFeatures.IHtmlGenerator",
+    "MemberId": "System.Threading.Tasks.Task RenderPartialViewAsync(Microsoft.AspNetCore.Mvc.Rendering.ViewContext viewContext, System.String partialViewName, System.Object model, Microsoft.AspNetCore.Mvc.ViewFeatures.ViewDataDictionary viewData, System.IO.TextWriter writer)",
+    "Kind": "Addition"


Adding something to an interface seems like an impossible change to make before 3.0. What makes this 🆗?

dougbu · 2017-11-29T05:58:17Z

src/Microsoft.AspNetCore.Mvc.ViewFeatures/ViewFeatures/DefaultHtmlGenerator.cs

-            {
-                throw new ArgumentNullException(nameof(antiforgery));
-            }
-
            if (optionsAccessor == null)
            {
                throw new ArgumentNullException(nameof(optionsAccessor));


Rewriting these null checks seems unnecessary. But, being inconsistent about it is Just Wrong:tm:

Also, did we change our mind about throw expressions? They were against our unofficial guidelines at one point.

NTaylorMullen

Should also remove breaking changes.

NTaylorMullen · 2017-11-29T19:20:27Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+                    ViewData,
+                    writer);
+
+                output.SuppressOutput();


This will nuke any other content added by other TagHelpers. If someone wants to add anything to the partial TagHelper i'd say let them. Instead of suppressing output just set the TagName to null to work nicely with anyone else.

NTaylorMullen · 2017-11-29T19:21:14Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        public ViewDataDictionary ViewData { get; set; }
+
+        [ViewContext]
+        public ViewContext ViewContext { get; set; }


[HtmlAttributeNotBound]

pranavkm

🆙 📅

dougbu · 2017-11-29T20:59:25Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        /// <summary>
+        /// A model to pass into the partial view.
+        /// </summary>
+        public object Model { get; set; }


Missed this before. Why isn't this

[HtmlAttributeName(ForAttributeName)] public ModelExpression For { get; set; }

I checked with @DamianEdwards and he doesn't think this should be a ModelExpression. He was fine leaving this property as is.

dougbu · 2017-11-29T21:09:30Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+                // Determine which ViewData we should use to construct a new ViewData
+                var baseViewData = ViewData ?? ViewContext.ViewData;
+
+                var newViewData = new ViewDataDictionary<object>(baseViewData, Model);


Overloads of the PartialAsync(...) and RenderPartialAsync(...) helpers default the model they use to htmlHelper.ViewData.Model. This instead defaults to null, which is unexpected. Should do something similar to the HTML helpers here.

Always use ViewContext.ViewData.Model since setting the ViewData property should not change the Model property's default.

dougbu · 2017-11-29T21:12:16Z

test/Microsoft.AspNetCore.Mvc.TagHelpers.Test/PartialTagHelperTest.cs

+            var tagHelper = new PartialTagHelper(viewEngine.Object, bufferScope)
+            {
+                Name = partialName,
+                Model = model,


Add tests where Model (now For) isn't set or is explicitly set to null but a view is found.

dougbu · 2017-11-29T21:15:47Z

test/WebSites/HtmlGenerationWebSite/Views/HtmlGeneration_Home/ProductListUsingTagHelpers.cshtml

+        foreach (var product in Model)
+        {
+            @* Update HtmlFieldPrefix so generated for, id and name attribute values are correct. *@
+            ViewData.TemplateInfo.HtmlFieldPrefix = fieldPrefix + string.Format("[{0}]", index++);


This wouldn't be necessary when using asp-for="[index++]".

dougbu · 2017-11-29T21:19:58Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+            }
+        }
+
+        private async Task RenderPartialViewAsync(TextWriter writer)


Just a suggestion: Could make this tag helper an HtmlHelper subclass which implements ITagHelper. Would have to make ViewContext a new property that calls Contextualize(...) in its setter but that should be the primary wrinkle.

Forgot the punchline: Then could use RenderPartialCoreAsync(...).

The sample @DamianEdwards has essentially DIs a IHtmlHelper, Contextualizes it and then renders the view using it. That kinda felt odd since we don't have any other use cases of doing this in any other tag helper. Given we already have a similarish copy of the code in PartialViewExecutor, I didn't think it was terrible to have yet another copy of it here.

Fixes #5916

pranavkm · 2017-11-30T17:20:29Z

🆙 📅

Eilon · 2017-11-30T20:14:26Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+        }
+
+        /// <summary>
+        /// The name of the partial view used to create the HTML markup.


Does this do just view "names" (e.g. _LoginPartial) or does it also do paths? Do we need to be clear about this in the docs?

I didn't see any tests using a path, but would it work? And if paths aren't supported:

Should they be?

The docs need to be clearer about what a "partial view name" is (perhaps by giving an example)

Looks like these properties are handled identically to e.g. viewName parameters in Controller.View(...) methods. Those are generally documented as The name of the view that is rendered to the response.. Same for the corresponding result properties e.g. ViewResult.ViewName.

If we need more documentation about what names, paths, et cetera mean, suggest we handle that in a separate issue. For now, let's be consistent.

It does path - https://github.com/aspnet/Mvc/pull/7089/files#diff-883159b6acb104f0fb64afa28c27a9ccR2 . The name I think comes from the IHtmlHelper docs. I'd be happy to update it if you have suggestions

Oh, and "partial view name" doesn't appear anywhere. Isn't "name of the partial view" clear enough (assuming we stick with just "name" for now)?

For now, let's be consistent.

I'd rather have correct information than have consistent-but-useless information

Oh, and "partial view name" doesn't appear anywhere.

These mean the same thing so I don't think this is critical.

...

So, let's come up with some good wording here and then apply elsewhere.

How 'bout "The name or path of the [partial] view that is rendered to the response." We do lots of shenanigans under the covers handling absolute and relative paths as well as distinguishing paths from names. But, describing exactly how we choose to interpret "name" versus "path" doesn't help much -- especially because it's specific to the RazorViewEngine.

I'm not positive what extra words we need to inform readers what distinguishes a "partial" view. It's a view found with isMainPage: false but the interfaces say only that isMainPage "Determines if the page being found is the main page for an action.". The details are again specific to the IViewEngine implementation. (For RazorViewEngine, this parameter mainly controls whether or not _ViewStart.cshtml files are used.)

dougbu

LGTM but get final word from @Eilon on the doc comments.

dougbu · 2017-11-30T21:07:18Z

src/Microsoft.AspNetCore.Mvc.TagHelpers/PartialTagHelper.cs

+                if (For?.Name != null)
+                {
+                    newViewData.TemplateInfo.HtmlFieldPrefix = newViewData.TemplateInfo.GetFullHtmlFieldName(For.Name);
+                }


Could move everything except var partialViewContext = ...; outside the using. Up to you.

pranavkm requested a review from dougbu November 29, 2017 01:55

ajaybhargavb reviewed Nov 29, 2017

View reviewed changes

dougbu suggested changes Nov 29, 2017

View reviewed changes

NTaylorMullen suggested changes Nov 29, 2017

View reviewed changes

pranavkm force-pushed the prkrishn/partial-tag-helper branch from 3002f4f to 092c3db Compare November 29, 2017 20:16

pranavkm commented Nov 29, 2017

View reviewed changes

dougbu suggested changes Nov 29, 2017

View reviewed changes

Add a <partial /> tag helper

6ed589f

Fixes #5916

pranavkm force-pushed the prkrishn/partial-tag-helper branch from 092c3db to c6ea066 Compare November 30, 2017 17:20

More tests, cleanup etc

e5110fe

pranavkm force-pushed the prkrishn/partial-tag-helper branch from c6ea066 to e5110fe Compare November 30, 2017 17:22

Eilon suggested changes Nov 30, 2017

View reviewed changes

dougbu approved these changes Nov 30, 2017

View reviewed changes

pranavkm closed this Dec 1, 2017

pranavkm deleted the prkrishn/partial-tag-helper branch December 1, 2017 01:10

Add a <partial /> tag helper #7089

Add a <partial /> tag helper #7089

Conversation

pranavkm commented Nov 29, 2017

ajaybhargavb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NTaylorMullen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment