fix: [SPA-1991] Image deep binding for custom components

[loweisz]

Purpose

Looks like deep binding with custom components never worked as we do the value-creating differently for the build in component to leverage the image optimization options.

The path was never resolved when including a deep binding to an asset.

As the asset is already there we can safely just get the url and return it to the component for custom components.

Note: We could consider also introducing the image optimization options to custom components as well.

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
experience-builder-test-app	✅ Ready (Inspect)	Visit Preview	Apr 11, 2024 2:56pm

[dimitrycf]

Whilst testing, I found this case:

Before:

• customer creates developer-component <MyImage heading:string mySrc:Media/>
• customer can use this component on canvas
• customer can bind to mySrc assets.file field
• customer can bind to mySrc any text field of an entry (eg. which contains url eg. txtImageUrl)
• BROKEN: customer can deep-bind to mySrc asset via L2 reference myEntry->myPicture->file

After:

• customer creates developer-component <MyImage heading:string mySrc:Media/>
• customer can use this component on canvas
• customer can bind to mySrc assets.file field
• NOT SUPPORTED ANYMORE: customer can bind to mySrc any text field of an entry (eg. which contains url eg. txtImageUrl)
• FIXED: customer can deep-bind to mySrc asset via L2 reference myEntry->myPicture->file

Taking into account the case marked with "NOT SUPPORTED ...", I have a question:

• [ ] do we want to allow users to bind Media to Text fields? (eg. typical use case would be to bind to text fields holding URLS eg. MyRecipe->txtImageUrl ) ?

Because if we DO want to support such use case:

• we need to fix this PR to enable it

Because if we DO NOT want to support such use case:

• we need to disallow clicking on Text fields when attempting to bind to Media (eg. make them disabled)

[dimitrycf]

So I have tested the core operations and they seem to work, but I also want to test how this may affect background image.

https://www.loom.com/share/491bb1e95baa45c3aa897333a4fc6ff2

[loweisz]

@dimitrycf Good catch, I strongly think we should support making it possible to bind to text fields as well, so I will try to fix this as well in this PR

[Spring3]

My only request would be to please add some tests (e2e?) to have this feature covered for future changes

[loweisz]

My only request would be to please add some tests (e2e?) to have this feature covered for future changes

@Spring3 I guess we would need to implement that on the web app side, which will always only test the version that is installed in the test app. Would that be sufficient in your opinion?

[Spring3]

My only request would be to please add some tests (e2e?) to have this feature covered for future changes

@Spring3 I guess we would need to implement that on the web app side, which will always only test the version that is installed in the test app. Would that be sufficient in your opinion?

yes, sorry, I got lost in the PRs I took a look at today and assumed that this one is also open for the web app. Yes, a test on the web app side would be nice to have 👍

[loweisz]

@dimitrycf I think I have adressed now all use cases, can you check if now everything works for you?

[loweisz]

I read a bit more code and it turns out that transformMedia() is only called from one place in the code, and it only accepts an asset as it's argument. And I was assuming that assets always have .file field...

callsite (note that if-statement which checks guards against Entry )

@dimitrycf Yes, but this check is BEFORE the transformMedia. What would you suggest doing, I think I don't really understand what you want me to tell here :D

[dimitrycf]

Also I would vote for leaving the <ImageTestComponent> in (this is a hostApp from within SDK and it's meant for internal usage and testing 🤔 , right? or is it used by any external parties? )

(if this were colorful coin then probably it would be annoying)