ImageTagHelper #2516
ImageTagHelper #2516
Conversation
|
Hi @dpaquette, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution! TTYL, DNFBOT; |
|
What's the deal with @dnfclas ? I signed my cla before submitting this PR |
|
@dpaquette We just changed the repos from MS Open Tech to .NET Foundation so there's a different CLA. The new CLA is simpler than the old one and so should only take a few moments to re-sign. Apologies for the extra bit of work! |
|
@dpaquette, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
| /// If <c>true</c> then a query string "v" with the encoded content of the file is added. | ||
| /// </remarks> | ||
| [HtmlAttributeName(FileVersionAttributeName)] | ||
| public bool? FileVersion { get; set; } |
There was a problem hiding this comment.
IncludeFileVersion? Or maybe AppendFileVersion? The current name makes it sound like it is a file version (e.g. you'd set asp-file-version="1.23.4" or something).
There was a problem hiding this comment.
I agree that AppendFileVersion or AddFileVersion would make more sense. This is modeled off the Link and Script tag helpers. If we change the name here then it would need to be changed on both those tag helpers too.
There was a problem hiding this comment.
Why nullable? It's in the TargetElement so it'll always be provided.
|
@Eilon I simplified the ImageTagHelper class by removing the Mode and Mode Detection code since this was no longer needed. Not sure what we should do about the naming of the asp-file-version attribute. |
| private FileVersionProvider _fileVersionProvider; | ||
|
|
||
| /// <summary> | ||
| /// Src of the image. |
|
⌚ |
|
@dougbu Simplified implementation and updated tests as per your comments |
| using Microsoft.Framework.Runtime; | ||
|
|
||
| using Moq; | ||
| using Xunit; |
There was a problem hiding this comment.
Please remove and sort usings. That should also clean up the extra blank line.
|
⌚ |
| { "data-extra", new HtmlString("something") }, | ||
| { "title", new HtmlString("Image title") }, | ||
| { "src", "testimage.png?v=f4OxZX_x_FO5LcGBSKHWXfwtSx-j1ncoSt3SABJtkGk" }, | ||
| { "asp-file-version", "true" } |
There was a problem hiding this comment.
in realistic scenario asp-file-version would not appear in the Attributes collection after processing
|
|
||
| private static TagHelperContext MakeTagHelperContext( | ||
| TagHelperAttributeList attributes = null, | ||
| string content = null) |
There was a problem hiding this comment.
Simplify method signature: No need for attributes to be optional. No need for content at all.
|
⌚ very close |
|
@dougbu @NTaylorMullen Thanks for the review. I have updated based on your feedback. In retrospect, basing this implementation on the Link and Script Tag Helpers was probably a bad idea 😄 |
| using System.IO; | ||
| using System.Linq; | ||
| using System.Text; | ||
| using System.Threading.Tasks; |
There was a problem hiding this comment.
Sorry, should have mentioned that we use the old VS default here: System namespaces first.
|
⌚ |
| @@ -47,6 +47,8 @@ public class MvcTagHelpersTest | |||
| [InlineData("Link", null)] | |||
| // Testing the ScriptTagHelper | |||
| [InlineData("Script", null)] | |||
| // Testing the ImageTagHelper | |||
| [InlineData("Image", null)] | |||
There was a problem hiding this comment.
Odd that MvcTagHelpersWebSite.MvcTagHelper_Home.Image.html shows up here as a binary file. Anything unusual about it on your end? (Content looks fine when I click on View.)
There was a problem hiding this comment.
Looks line an encoding problem. I will try saving as UTF-8
|
Please rebase your branch to latest dev then squash your commits down to one ( |
| /// <inheritdoc /> | ||
| public override void Process(TagHelperContext context, TagHelperOutput output) | ||
| { | ||
| if (FileVersion == true) |
|
|
An ImageTagHelper that supports cache busting by appending a file version hash to the image src attribute [Resolves #2249] Code cleanup
|
@dougbu @NTaylorMullen Thanks guys. I did a rebase to latest dev and squashed my commits. |
|
BTW I logged #2540 to track changing the name of the |
|
ab4d2ee Thanks for your contribution! |
An ImageTagHelper that supports globbed src paths and cache busting file
version hash
[Resolves #2249]