- Issue created by @lauriii
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Please share the SDC with which you tested this. Otherwise we have to waste time figuring out how to reproduce this.
- 🇫🇮Finland lauriii Finland
Tested this with the hero component from SDDC: https://git.drupalcode.org/project/demo_design_system/-/tree/1.0.x/stars....
- 🇬🇧United Kingdom longwave UK
Picking this up as I've been solving other image issues in 📌 ComponentMetadataRequirementsChecker::check() should validate that the example(s) actually match the JSON schema Active
- 🇺🇸United States tedbow Ithaca, NY, USA
Chatted with @longwave. I am going to take a look at this since he is on another issue
- 🇺🇸United States tedbow Ithaca, NY, USA
I am wondering if this is front-end issue. I am not super familiar with how the front-end sends back the form inputs but this is what I am seeing using the
optional-props-image
component I added in the MR- Add the component to the canvas
- open the edit form
- start to type in the heading
- the image goes away in the canvas
Between step 3 and 4, I debugged the call to
\Drupal\experience_builder\Controller\ApiLayoutController::patch()
The body of the request has model->source->heading and model->resolved->heading but 'image' is not under either shouldn't it be sent back under 'source' even though it is empty?
Looking at the value in the client form of
edit-form-xb-props
I see{ "name": "XB test SDC with optional text and image", "resolved": { "heading": { "value": "Heading the right direction?" } }, "source": { "heading": { "expression": "ℹ︎string␟value", "sourceType": "static:field_item:string", "value": { "value": "Heading the right direction?" } }, "image": { "required": false, "jsonSchema": { "title": "image", "type": "object", "required": ["src"], "properties": { "src": { "title": "Image URL", "type": "string", "format": "uri-reference", "pattern": "^(/|https?://)?.*\\.(png|gif|jpg|jpeg|webp)(\\?.*)?(#.*)?$" }, "alt": { "title": "Alternative text", "type": "string" }, "width": { "title": "Image width", "type": "integer" }, "height": { "title": "Image height", "type": "integer" } } }, "sourceType": "static:field_item:entity_reference", "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟entity␜␜entity:file␝uri␞␟url,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}", "sourceTypeSettings": { "storage": { "target_type": "media" }, "instance": { "handler": "default:media", "handler_settings": { "target_bundles": { "image": "image" } } } } } } }
So it does have the info for image. should it be sending back what it has?
- 🇺🇸United States tedbow Ithaca, NY, USA
Manually testing the MR now I can get to not lose the default image when changing the heading property.
My solution was to set the props that were missing from the client in
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::clientModelToInput
Not knowing this part of the code it seems reasonable to allow the client the skip props that don't have a value, since the server can figure out this info.
Will see if this breaks other tests
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@tedbow: when it's green, can you please mark and assign to @longwave? 🙏 He knows this bit best.
- 🇬🇧United Kingdom longwave UK
This seems related to Wim's analysis in #3507929-25: Allow adding "image" props in the code component editor → .. not sure the fix is quite right but it's in the right location, will try to look later on today.
- 🇬🇧United Kingdom longwave UK
Added a test for this and fixed it to only apply to object shapes; this works but it feels wrong? Also I don't like the way the fallbacks are checked both before and then after the inputs are set up, but too tired now to figure out how to refactor it nicely. The test passes locally though!
- 🇬🇧United Kingdom longwave UK
Wondering if this it the right way at all, maybe we should be altering the SDC component info much earlier to remap the example/default path instead of trying to map it in via a prop source.
- 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
I've been testing https://github.com/phenaproxima/xb-demo and ran into this issue and using the MR on my local fixed this issue.
- 🇺🇸United States darren oh Lakeland, Florida
darren oh → made their first commit to this issue’s fork.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
wim leers → changed the visibility of the branch 3508077-release-patch to hidden.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
#15: AFAICT this is hugely overlapping with #3509608-9: Unable to save an optional image with its default value, because no image media is selected → ?! I bet you write in #9 it feels wrong for lots of reasons, and I articulated those reasons in #3509608-9?! 😅
- First commit to issue fork.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I bet 📌 Split model values into resolved and raw Active will help with this.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Unfortunately, 📌 Split model values into resolved and raw Active hasn't been completed yet, even though it would have removed the need for this work-around. @longwave confirmed at #3493943-20: Split default values into resolved and raw → that that is the proper fix.
So, to ease the pain during DrupalCon (next week!), we intend to land this work-around, knowing full well that #3493943 will again remove it.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Going ahead and merging this to fix this ahead of DrupalCon next week.
But https://git.drupalcode.org/project/experience_builder/-/merge_requests/7... identified that this is actually being fixed in the wrong place: it's not broken on the server side, but on the client side: https://git.drupalcode.org/project/experience_builder/-/merge_requests/7....
I'm assuming 📌 Split model values into resolved and raw Active will fix that, too.
-
wim leers →
committed 40f251b3 on 0.x authored by
tedbow →
Issue #3508077 by wim leers, tedbow, longwave, lauriii: Default image...
-
wim leers →
committed 40f251b3 on 0.x authored by
tedbow →