Skip to content
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

Introduce Taghelper #290

Merged
merged 37 commits into from
Mar 25, 2023
Merged

Introduce Taghelper #290

merged 37 commits into from
Mar 25, 2023

Conversation

JimBobSquarePants
Copy link
Member

@JimBobSquarePants JimBobSquarePants commented Oct 5, 2022

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following matches the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Adds a new ImageSharpTaghelper that greatly simplifies URL generation (including automatic generation of HMAC keys).
The TagHelper is designed to be inheritable allowing the addition of custom commands.

Example usage (note, the syntax highlighter here does not do the TagHelper justice. The property values are all strong typed)

<img src="sixlabors.imagesharp.web.png"
     width="300"
     height="200"
     imagesharp-width="600"
     imagesharp-height="400"
     imagesharp-rmode="ResizeMode.Pad"
     imagesharp-rcolor="Color.LimeGreen" />

The TagHelper will only intitialize when both the following criteria are met:

  1. The TagHelper is explicitly referenced in _ViewImports.cshtml via @addTagHelper *, SixLabors.ImageSharp.Web
  2. The src and imagesharp- prefixed HTML attributes are declared.

@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

Merging #290 (e057f93) into main (35e1ae2) will increase coverage by 0%.
The diff coverage is 87%.

@@         Coverage Diff          @@
##           main   #290    +/-   ##
====================================
  Coverage    85%    85%            
====================================
  Files        77     83     +6     
  Lines      2199   2375   +176     
  Branches    313    346    +33     
====================================
+ Hits       1871   2026   +155     
- Misses      235    245    +10     
- Partials     93    104    +11     
Flag Coverage Δ
unittests 85% <87%> (+<1%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/ImageSharp.Web/Middleware/ImageContext.cs 80% <ø> (ø)
...rc/ImageSharp.Web/RequestAuthorizationUtilities.cs 86% <50%> (ø)
src/ImageSharp.Web/CaseHandlingUriBuilder.cs 88% <71%> (ø)
...rc/ImageSharp.Web/TagHelpers/HmacTokenTagHelper.cs 74% <74%> (ø)
src/ImageSharp.Web/TagHelpers/ImageTagHelper.cs 91% <91%> (ø)
...DependencyInjection/ServiceCollectionExtensions.cs 100% <100%> (ø)
src/ImageSharp.Web/Format.cs 100% <100%> (ø)
src/ImageSharp.Web/FormatCommand.cs 100% <100%> (ø)
src/ImageSharp.Web/HMACUtilities.cs 86% <100%> (ø)
.../ImageSharp.Web/Middleware/ImageSharpMiddleware.cs 81% <100%> (-1%) ⬇️
... and 2 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@ronaldbarendse ronaldbarendse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a really cool feature and will make it a LOT easier to generate the correct URLs 🙌🏻

I've added some comments/suggestions and just having support for <img src="" /> is already great for the initial release. Adding srcset support for both img and picture/source elements will probably be out of scope for this PR, since there's a lot of ways you can render responsive images.

src/ImageSharp.Web/Format.cs Outdated Show resolved Hide resolved
Comment on lines 38 to 39
private const string WidthAttributeName = ResizeWebProcessor.Width;
private const string HeightAttributeName = ResizeWebProcessor.Height;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both width and height aren't prefixed, causing <img src="test.jpg" width="500" height="500" /> to result in adding commands to the URL, even though that might not be needed/intended...

I would recommend prefixing these as well and copying the width/height to the tag attributes (if they aren't explicitly set already).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was actually a conscious decision. My thought process was that it’s bad practice to use width/height attributes with unscaled content. Happy to be proven that this is a bad idea though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main issue is that this will result in changing all existing image URLs to include the width/height (and HMAC token) commands by simply adding @addTagHelper *, SixLabors.ImageSharp.Web to your _ViewImports.cshtml. It will also process and cache these images, even though they will most likely already be the correct size (otherwise they would get scaled or distorted, which might even be done on purpose, e.g. when providing a high DPI version, upscaling, etc.).

Besides that, you might want to use a custom command to do the resizing (like Umbraco does with crop coordinates in the CropWebProcessor) and omit the width command from the URL while still explicitly setting the width attribute on the img tag. Another case might be when you disable the auto-orient feature and therefore have to swap the width/height commands.

You can avoid this by also using prefixes for these commands, so <img src="test.jpg" imagesharp-width="500" imagesharp-height="500" /> will be rendered as <img src="test.jpg?width=500&height=500" width="500" height="500" />. This will still set the attributes, but only resize the image when explicitly set. This will also allow adding high DPI versions by using <img src="test.jpg" width="250" height="250" imagesharp-width="500" imagesharp-height="500" />.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. You've convinced me. Explicit it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify the new behavior.

  • We only resize if imagesharp-width or imagesharp-height are explicitly set.
  • We only set the width or height attributes if there are no attributes present.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh? Why the hell would someone write a command like that mixed in with the helper syntax? They do that and they’re on their own cos it’s bloody stupid

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the helper, don’t use the helper fine by me but don’t expect me to support stupid behaviour.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not have explained it clearly enough 😇 There's a couple of benefits when separating the HMAC token generation into a separate tag helper:

  1. You don't have to create your own tag helper (inherited from ImageTagHelper) to add a custom command (e.g. imagesharp-cropcoordinates) and have it included in the HMAC token, as you can then manually include that command in the image URL/src attribute;
  2. The HMAC tokens can easily be added to other elements and attributes (e.g. img[srcset] and source[srcset]), without having to implement new tag helpers to generate the image URLs for these elements/attributes;
  3. Existing image URLs that contain valid commands (e.g. returned from a CMS) can be augmented with additional commands and/or the HMAC token.

Maybe some code might help to explain it, so please have a look at these changes: c72ee4a (I've added some other suggestions to my fork, see branch comparison).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ronaldbarendse if you're still keen, I'd like to revisit your suggestion to split out the helper?

Copy link
Member Author

@JimBobSquarePants JimBobSquarePants Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I ran with it.

I have implemented pretty much everything you suggested with a few implementation tweaks. You know better what downstream consumers will require than I.

HMAC is now the separate helper you designed and requests with no commands no longer require HMAC - Access for them can be controlled via Authorization middleware.

src/ImageSharp.Web/ImageSharpTagHelper.cs Outdated Show resolved Hide resolved
src/ImageSharp.Web/ImageSharpTagHelper.cs Outdated Show resolved Hide resolved
@JimBobSquarePants JimBobSquarePants marked this pull request as ready for review October 23, 2022 06:34
@JimBobSquarePants JimBobSquarePants changed the title WIP - Introduce Taghelper Introduce Taghelper Oct 23, 2022
@JimBobSquarePants JimBobSquarePants added this to the v2.1.0 milestone Oct 23, 2022
src/ImageSharp.Web/Format.cs Outdated Show resolved Hide resolved
src/ImageSharp.Web/Format.cs Outdated Show resolved Hide resolved
src/ImageSharp.Web/Format.cs Outdated Show resolved Hide resolved
@JimBobSquarePants JimBobSquarePants merged commit cc8cc49 into main Mar 25, 2023
@JimBobSquarePants JimBobSquarePants deleted the js/taghelper branch March 25, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants