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

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

Open
mistyn8 opened this issue May 19, 2023 · 0 comments

Comments

@mistyn8
Copy link

mistyn8 commented May 19, 2023

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant