BUGFIX: Respect aspect ratio

[manuelmeister]

Use targetWidth and targetHeight for aspect ratio calculation when both are set. Otherwise this leads to unwanted results. We observed this behaviour with srcset and 1x, 2x and imageSource.withHeight(40, true)

[mficzel]

I would suggest to use withDimensions if you specify both values as with withWidth or withHeight it is pretty unclear what to expect if they are called after each other.

I think following code in https://github.com/sitegeist/Sitegeist.Kaleidoscope/blob/main/Resources/Private/Fusion/Prototypes/Image.fusion :

imageSource = ${props.imageSource}
[email protected] = ${props.width ? value.withWidth(props.width) : value}
[email protected] = ${props.height ? value.withHeight(props.height) : value}

probably should be more like

imageSource = ${props.imageSource}
[email protected] = ${(props.width && !props.height) ? value.withWidth(props.width) : value}
[email protected] = ${(props.height && !props.width) ? value.withHeight(props.height) : value}
[email protected] = ${(props.width && props.height) ? value.withDimensions(props.width, props.height) : value}

also the whole thing should probably use @Private instead of the nested Component nowadays.

[mficzel]

If @Private is used this could also look more like

@private.imageSource = Neos.Fusion:Case {
        @if.hasImageSouzrce = ${props.imageSource}
	withDimensions {
		condition = ${props.width && props.height}
		renderer = ${props.imageSource.withDimensions(props.width, props.height) }
	}
	withWidth {
		condition = ${props.width}
		renderer = ${props.imageSource.withWidth(props.width, props.height) }
	}
	withHeight {
		condition = ${props.height}
		renderer = ${props.imageSource.withHeight(props.width, props.height) }
	}
	default {
		condition = true 
		renderer = ${props.imageSource}
	}
}

[manuelmeister]

I'm not sure I understand. I'm using imageSource.withHeight(40, true) outside the Kaleidoscope package to render an icon with max height 40px and its source aspect ratio. I don't want the picture to be cut, just resized to height 40px (or 80px for 2x). I don't know the aspect ratio of the icon, as this is provided by content editors and I don't restrict the image aspect ratio in the cms.

The true boolean is set to keep the aspect ratio:

Sitegeist.Kaleidoscope/Classes/Domain/AbstractScalableImageSource.php

Lines 55 to 61 in df39bd6

	/**
	* @param int|null $targetHeight
	* @param bool $preserveAspect
	*
	* @return ImageSourceInterface
	*/
	public function withHeight(int $targetHeight = null, bool $preserveAspect = false): ImageSourceInterface

[manuelmeister]

I'm doing this:

[email protected] = ${value ? value.withHeight(40, true) : value }

renderer = afx`
           <Sitegeist.Kaleidoscope:Image
                imageSource={props.imageSource}
                srcset="1x, 2x, 3x"
        />
`

This should result in 3 images being generated for height 40, 80, 120 and their aspect depended width.

[mficzel]

Now i got it ... fair point. Makes total sense.

[manuelmeister]

also the whole thing should probably use @Private instead of the nested Component nowadays.

I'm not sure if Kaleidoscope should already move to @private in general as this would increase the min. requirement for neos to v8.3.

I really love @private and often it is more clear, but I think this would probably be a breaking change.

[mficzel]

If a feature release adjusts the requirement to 8.3||9.0 I see no problem. Existing projects can always use the version they already have. New projects should be 8.3 or 9 anyways.