!!! FEATURE: Variant preset support

[kdambekalns]

Provides a new property variantPreset in the AssetImageSource and
DummyImageSource Fusion prototypes. The expect a string giving the
preset identifier and preset variant name separated by ::, like:

Acme.Com:Component.Teaser.Hero::landscape

In addition this adds a new method useVariantPreset() to the
ImageSourceHelperInterface that can be used to specify a variant to
use based on a configured asset variant preset:

useVariantPreset('Acme.Com:Component.Teaser.Hero', 'landscape')

That variant (if existing) of an asset will be used as base before any
further adjustments are applied.

Furthermore this changes the API for using thumbnail presets:

• renames applyPreset() to applyThumbnailPreset() in
  ImageSourceHelperInterface
• renames preset to thumbnailPreset in Fusion prototypes

The key preset in Fusion is still used if thumbnailPreset is not
given to provide b/c.

Resolves #12

[kdambekalns]

Here we go, this works for my current use-case.

I thought about adding a code migration to replace applyPreset with applyThumbnailPreset to ease migration even more. Replacing preset with thumbnailPreset seems to "dangerous" (given the rather unspecific term "preset"), so I opted for supporting it further.

/cc @robertlemke @kitsunet

[mficzel]

The full review will take time, you adjusted quite a bit

[kdambekalns]

The full review will take time, you adjusted quite a bit

I tried to be a good citizen and split things into seperate commits for easier review. In any case, feel free to inquire about changes you consider strange! And if you'd simply say "the CGL is different for this package," I'll gladly undo style changes.

I'd recommend keeping the typing stuff, though. ;)

[mficzel]

Ahh i did not see that this are multiple commits. I like the change and also the Adjustments.
It will take some time until this can be merged as #13 has to be merged and released first since this it work with 4.3 aswell and this change is for neos ^5.0

Only after that i can review this one, i will give feedback on the api now so you can require your branch for now and change to the released version later.

[kdambekalns]

Hm, I don't understand why #13 has to be merged first? I built this in a project using Neos 4.3, and would love to be able to use it with that "officially"…

Or is it because it's breaking and you want to limit the number of major releases?

[mficzel]

@kdambekalns i finalized the format support for neos 4.3 and released it. Your PR should be rebased on that.

[mficzel]

Other than the required rebase i am fine with this api wise. Thanks a lot for the PR. Everything else can be fixed in a bugfix version. I have no setup at hand with preset variants to actually test.

I would release this as major version as this requires Neos 5 and renames the existing preset property.

One question remains: What will happen if multiple usePresets() are chained or an imageSourceHelper is passed with selected preset and then the presentation selects another one.

[kdambekalns]

I would release this as major version as this requires Neos 5 and renames the existing preset property.

I will happily rename thumbnailPreset back to preset, as I actually need this for a project based on Neos 4.3. That also means, I know it's working with 4.3 – what makes you think otherwise?

What will happen if multiple usePresets() are chained

Since it doesn't make sense to use multiple presets, the last one wins. (Like with the other helper methods, AFAICS.)

or an imageSourceHelper is passed with selected preset and then the presentation selects another one.

Huh. No idea. :) Didn't try that, I only use it in a presentational component so far. I'd think, later specification wins (again.)

[kdambekalns]

Then again, it changes the ImageSourceHelperInterface - do you consider that part of the public API? If so, it'll be a breaking change either way.

But my main concern is: Neos 4.3 needs to be supported.

[kdambekalns]

See kdambekalns@4815977 for a B/C version of this (except for the ImageSourceHelperInterface change…)

[mficzel]

@kdambekalns i just assumed this would be for 5.0 as the pr came in just after the api method was added and you mentioned this pr as usecase. Offcourse we can release this for 4.3 if it works there aswell. I would just release this as Kaleidoscope 5 because of the attribute renaming.

[kdambekalns]

Well, for 5.0 one method in this PR could be dropped, but now it's there and does no harm on 5.0 either. So, feel free to merge and release this… 🙃

[mficzel]

@kdambekalns i do not understand fully why the dummyImageController has to be aware of the variants. As the target image dimensions are already defined in the imageSourceHelpers. Can you explain this? I would expect the dummyImageSource handling this entirely without any modification (other than cleanup) on the dummyImageController.

[kdambekalns]

@mficzel Because variant presets are more than just width/height (as with thumbnails.) They can adjust the quality, transform to grayscale, crop intelligently, … so they need to be applied to the "source" image, and that is created in the dummy image controller. So we need to apply those adjustments "in place". For a "real" asset source, the variant that comes in already has those applied, so it's only any "extra" changes that need to be done (like resize the chose variant).

Does that explain it?

[mficzel]

@kdambekalns is'nt this a bit overengineering and also counter intuitive i would expect the abstractImageSourceHelper to extract targetWidth and targetHeight from the variantConfiguration on useVariantPreset and the assetImageSourceHelper to additionally return a clone with the chosen variant as asset.

Also if the variant-transformations are applied on the dummy images in the controller the transformations that modify the size of the dummy image will have an effect on dummy images but not on assets since there Kaleidoscope has the last word.

[mficzel]

It would also could sense to keep the dimensions entirely out of this and make useVariantPreset an empty operation for all helpers other than the assetImageSource. That way the dimension decisions would be entirely determined by the presentational component via setWidth, setHeight or setDimensions.

Or the solution in between, let useVariantPreset modify only the baseWidth and baseHeight of the dummyImageSource. That way dimensions defined explicitly would still always be applied.

[mficzel]

In afx würde ich das übrigens vor allem so nutzen wollen

<Sitegeist.Kaleidoscope:Source imageSource={props.imageSource.useVariantPreset('Acme.Com:Component.Teaser.Hero', 'narrow')} srcset="375px, 750px, 1125px" media="(max-width: 719px)" />

Wobei dein case oben so oder so funktioniert

[mficzel]

FYI: I merged the cleanup commit already to 4.0 and merged 4.0 back in here so this is now only about the variant preset changes. Please pull before working on this but we want to talk about this anyways on monday

[kdambekalns]

• AssetImageSource and DummyImageSource must adjust the baseWidth and baseHeight
• applyThumbnailPreset() must properly return the clone returned by setWidth() and or setHeight()
• actually applying the adjustments for a variant in the DummyImageController is useless: the resulting size needs to be known in the image source (and then passed to the controller), otherwise the "source image" will have the correct dimensions, but the "image source" never knew them… Thus the code has to be moved.

[larslehners]

Hey there, I've just testet this PR in a project and it works like a charme! Thanks a lot!!!

[kdambekalns]

I hope that does the job. Using the adjustments in the DummyImageSourceHelper is a bit cumbersome and weird, but seems to work that way.

[kdambekalns]

@mficzel When could you give this another look? If you need budget for this, get in touch with me…

[mficzel]

@kdambekalns i am on it but the misleading examples in the variants preset documentation where not that helpful neos/neos-development-collection#2755

[kdambekalns]

Well, there is neos/neos-development-collection#2716 as well…

[mficzel]

@kdambekalns In general i like this now and it works as expected with the following fusion component in styleguide and frontend:

prototype(Test:Image) < prototype(Neos.Fusion:Component) {
    @styleguide {
        props {
            imageSource = Sitegeist.Kaleidoscope:DummyImageSource
        }
    }
    imageSource = null
    renderer = afx`
        <picture>
            <Sitegeist.Kaleidoscope:Source imageSource={props.imageSource.useVariantPreset('Flownative.Demo:Preset1', 'square')} srcset="200w, 400w" sizes="100vw" media="(max-width: 799px)" />
            <Sitegeist.Kaleidoscope:Source imageSource={props.imageSource.useVariantPreset('Flownative.Demo:Preset1', 'wide')} srcset="600w, 900w, 1200w" sizes="100vw" media="(min-width: 800px)" />
            <Sitegeist.Kaleidoscope:Image imageSource={props.imageSource.setWidth(600)} />
        </picture>
    `
}

The one thing i am not sure about is the behavior when a variant does not exist. Currently the original variant will still be used with the original dimensions. In the context of kaleidoscope it may be useful to still enforce the dimensions in that case like the thumbnailPreset does.

That would require to move the fallback base dimension calculation up to the scalableAssetSource. What do you think?

[mficzel]

@kdambekalns i also pushed 2 small bugfixes so make sure to pull before continuing

[kdambekalns]

The one thing i am not sure about is the behavior when a variant does not exist. Currently the original variant will still be used with the original dimensions. In the context of kaleidoscope it may be useful to still enforce the dimensions in that case like the thumbnailPreset does.

True, that's replicating the behaviour of the media management in that regard (not available, use the original.) And while it might be nice to apply the size anyway, it would require to read the dimensions from the configured adjustments (because applying them would in fact create the missing variant, a rather strange side effect IMHO). That would in turn again mean only some are supported (those known to the implementation.) That's all quite a bit easier for the thumbnails…

Given that you have a simple workaround (apply size in Fusion after useVariantPreset) that keeps full control with the integration, I am not entirely convinced…

That would require to move the fallback base dimension calculation up to the scalableAssetSource. What do you think?

I think I don't understand that part of what you wrote… 🤦‍♂️ What "fallback base dimension calculation"?

[mficzel]

@kdambekalns i meant the calculation of the dimensions from the preset configuration as it is done for dummyImages.

We might use this as fallback when the variantPreset is not present. That would require moving this code from DummyImageSource up to the AbstractScalableImageSource.

I am not convinced that adding the dimensions via setWidth() and setHeight() again is a good solution for integrators. I sense integrators running into trouble.

[mficzel]

@kdambekalns i pushed a change that adjusts the code as i suggested above . Please take a look. If you agree to that i would be fine with it finally.

[kdambekalns]

Tested it again, works fine AFAICT…

[kdambekalns]

I'm fine with this, feel free to merge and release, @mficzel … thanks for you input!