Respect heights

[kahlan88]

• with media-item + ignore-crops flag - respect any height values passed in, including responsive heights This PR should help control rendered crops from our-img directly, instead of taking the original image's height ratio for crops (this is done using a new optional boolean/flag ignore-crops.

• with src - respect height values passed in When using src , the heights were being ignored automatically too. This change will respect any height props passed on <our-img>.

Solves https://github.com/umbraco-community/Our-Umbraco-TagHelpers/issues/65

[mistyn8]

not sure ignore crops is the correct naming.. or a new ignoreCrops param is requried? isn't it more a case of ..

• ImgWidth+ImgHeight is provided, then don't use the original aspect ratio of the image, crop to the new aspect ratio from these explicit dimensions
• ImgWidth only, then we should use the original aspect (current code) to calculate height
• ImgHeight only, then we should use the original aspect (currently missing) to calculate width

so need to look at if image height (ImgHeight) is provided for these use cases?

It would also apply that if ImgHeight and cropalias supplied then we should get an image based on the cropalias aspect and focal point but adjusted to the ImgHeight, currently it only allows ImgWidth control. For all three supplied cropalias, ImgHeight and ImgWidth that should perhaps give you the overridden aspect ratio but respect the focal point?

[kahlan88]

not sure ignore crops is the correct naming.. or a new ignoreCrops param is requried? isn't it more a case of ..

• ImgWidth+ImgHeight is provided, then don't use the original aspect ratio of the image, crop to the new aspect ratio from these explicit dimensions
• ImgWidth only, then we should use the original aspect (current code) to calculate height
• ImgHeight only, then we should use the original aspect (currently missing) to calculate width

so need to look at if image height (ImgHeight) is provided for these use cases?

It would also apply that if ImgHeight and cropalias supplied then we should get an image based on the cropalias aspect and focal point but adjusted to the ImgHeight, currently it only allows ImgWidth control. For all three supplied cropalias, ImgHeight and ImgWidth that should perhaps give you the overridden aspect ratio but respect the focal point?

Good question! I didn't want to change the existing logic too much (because we don't use crops,doesn't mean this logic doesn't suit others!)

If you think it's acceptable to change the code to always respect the height when provided - I'm happy to edit it to match that and remove the ignoreCrops flag.

[warrenbuckley]

@AndyBoot any thoughts on this as you done the work on our:img

[AndyBoot]

Thanks @warrenbuckley for tagging me in on this. I think this PR covers a few more scenarios which weren't originally anticipated. All looks good but as @mistyn8 pointed out the introduction of a new ignoreCrops may be overkill, especially when if we don't define a cropalias or width/height could be a better indicator to detect this. How are you getting on with the edit @kahlan88? Happy to test the code once ready!

[kahlan88]

efine a cropalias or width/height could be a better indicator to detect this. How are you getting on with the edit @kahlan88? Happy to test the code once ready!

I did think of doing it in the way you suggested initially, but opted for an option that won't be breaking for people when they update to the new version. As it is, it might change their crops without them realising.

I agree, though, that those params should be respected and ignoring crops implied. I will proceed with:

• replace ignoreCrops with ImgWidth and ImgHeight

@AndyBoot, I'm currently unable to work on it, but it's on my list. I'll tag you again when I've done the work.

[kahlan88]

@AndyBoot, @mistyn8, @warrenbuckley we've made some progress on this in https://github.com/umbraco-community/Our-Umbraco-TagHelpers/pull/69 @stayawakesteve took over my progress and addressed your comments. I just looked at it and happy with the work and it resolves height and aspect ratio issues we were experiencing.

Could you review that please and we can close this PR then? Thanks!