Skip to content

<Our-Img cropAlias={alias}> fails if no localcrops are set, but a global crop does exist, or if the crop is non existant #64

@mistyn8

Description

@mistyn8
if (!string.IsNullOrEmpty(ImgCropAlias))
{
      // The element contains a crop alias property, so pull through a cropped version of the original image
      // Also, calculate the height based on the given width using the crop profile so it's to scale
      imgSrc = MediaItem.GetCropUrl(width: (int)width, cropAlias: ImgCropAlias);
...
}

MediaItem.GetCropUrl(width: (int)width, cropAlias: ImgCropAlias); already deals with localcrops taking precedence over global so that's ok.. though we should guard against a non-existant crop too . if (!imgSrc.IsNullOrWhiteSpace()){...}

https://docs.umbraco.com/umbraco-cms/fundamentals/backoffice/property-editors/built-in-umbraco-property-editors/media-picker-3#using-crops

but then to calculate the relative aspect height to inject on the <img height=""/> we are only considering localcrop..

var cropWidth = MediaItem.LocalCrops.GetCrop(ImgCropAlias).Width;
var cropHeight = MediaItem.LocalCrops.GetCrop(ImgCropAlias).Height;
height = (cropHeight / cropWidth) * width

https://github.com/umbraco-community/Our-Umbraco-TagHelpers/blob/main/Our.Umbraco.TagHelpers/ImgTagHelper.cs#L167-L168

we should check that local crops exist and if not then fall back to global crops.
Also the width and height here come back as int... so our aspect calculation is then off, cast to double sorts that with a Math.Round() later on.

double cropWidth = MediaItem.LocalCrops?.GetCrop(ImgCropAlias)?.Width ?? MediaItem.Content.Value<ImageCropperValue>(Umbraco.Cms.Core.Constants.Conventions.Media.File)?.GetCrop(ImgCropAlias)?.Width ?? 0d;
double cropHeight = MediaItem.LocalCrops?.GetCrop(ImgCropAlias)?.Height ?? MediaItem.Content.Value<ImageCropperValue>(Umbraco.Cms.Core.Constants.Conventions.Media.File)?.GetCrop(ImgCropAlias)?.Height ?? 0d;
  if (cropWidth > 0 && cropHeight > 0)
  {
      height = (cropHeight / cropWidth) * width;
  }

...
#region Apply the width & height properties
  if (width > 0 && height > 0)
  {
      output.Attributes.Add("width", Math.Round(width));
      output.Attributes.Add("height", Math.Round(height));
  }

but then thinking about this a little.. MediaItem.GetCropUrl(width: (int)width, cropAlias: ImgCropAlias); has already done the heavy lifting for us (both in terms of local vs global crops and calculation of a relative height from the crop aspect ratio) and returns a url with the calculated height based on the width and cropAlias supplied
eg
/media/kfsjhtyb/702-1024x768.jpg?width=100&height=167&rnd=133289930878170000

so can we not just forgo all this recalculation and simply extract the height from the url?

 var q = HttpUtility.ParseQueryString(imgSrc.Split("?")[1] ?? string.Empty);
 height = double.TryParse(q["height"], out var cropHeight) ? cropHeight : 0d;

and we could do this just before the height attribute is written out, as we've used getCropUrl for cropAlias or defined width at this point (as simple image returns earlier)

If this seems sensible happy to generate a pr...

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions