- Issue created by @lauriii
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
We'll need to check if clicking the button triggers the appropriate Redux state changes.
- 🇺🇸United States bnjmnm Ann Arbor, MI
There's some trickiness here (surprise!) because all of the current redux-updating is based on changes to input values, but when an item is removed via media library, this is reflected in the form/ui by removing the correspnding hidden
<input />
that represents the selection.I have some ideas on how to deal with this...
- 🇺🇸United States bnjmnm Ann Arbor, MI
Current MR gets us closer to fixing it. Currently works best when the UI is loaded and the image prop already has media.
It is still buggy when new media is added and then removed. I assume it's because the component is now removed-element aware, but there isn't yet a full equivalent for additions
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@f.mazeikis re-discovered this same bug. @longwave is going to update the
media-library.cy.js
e2e test.During pair (triple?) debugging we discovered that the client-side model never got updated. Our going theory is that the
mediaSelection
client-side transform added in 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is not firing when removing an image. - First commit to issue fork.
- 🇬🇧United Kingdom longwave UK
Rebased and added assertions to media-library.cy.js to demonstrate the issue. The canvas starts with a sample image inserted, which we assert correctly, then the Remove button is clicked, which (I believe) should fall back to the example image for the component - but this doesn't happen at the moment so the test fails.
- 🇬🇧United Kingdom longwave UK
Removed the SDC changes, as per Wim we can use
image-optional-with-example-and-additional-prop
for testing here.There is something wrong with the changes to the frontend in this MR, as this now has the same result as 🐛 SDCs with optional images without examples cannot be placed Active when testing manually with
image-optional-with-example-and-additional-prop
- as soon as I select a valid media entity then the form errors out with a 400 error because the prop is missing. I've tried making some changes in the frontend code but I'm not sure what I'm doing or where it's going wrong so assigning to @bnjmnm for more investigation. - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
https://git.drupalcode.org/project/experience_builder/-/blame/0.x/ui/src... is the likely cause (disclaimer didn't check the MR)
When we click remove there's no new elements in the ajax response and hence nothing to trigger the model update via this event - Assigned to bnjmnm
- Status changed to Needs review
about 1 month ago 3:41pm 2 May 2025 - 🇫🇮Finland lauriii Finland
Tried to test this and it doesn't seem to be doing what's expected. The image persists but I'm also unable to make changes to other props after removing the image 🤔
- 🇺🇸United States bnjmnm Ann Arbor, MI
@lauriii Id like to try and reproduce your findings to narrow down what is going on, but it looks like the component you are testing with is called "Image with text", which is not one available in the experience builder codebase.
The MR was tested with the various image-including components provided by the
xb_test_sdc
module, as well as the standardimage
component. I'd like to see what is different about the one in #18 and figure out which side things need to be addressed. - 🇺🇸United States bnjmnm Ann Arbor, MI
The component used for the review in #18 is a custom code component. I think it's still worth reviewing with the included-with-experience-builder components the MR was tested with as it would be good to get feedback on the solution approach sooner than later.
Once I have access to & can successfully import the custom component from #18 I can see why that is running into problems, but that probably should preclude getting eyes on the solution that has solved the reported symptom for the components that are available to anyone with the Experience Builder codebase.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Also, with the current solution in the MR any removed image reverts to the example when one is available, otherwise it is removed from the preview. The IS suggests there might be interest in different logic depending on whether or not the image prop is required. That should be a feasible expansion of the current logic if needed.
- 🇫🇮Finland lauriii Finland
Tested this with the built in components and couldn't reproduce #18 🐛 Default image is not loaded after removing media Active with them. I attached config for the component that allows you to reproduce this.
When remove is clicked on an optional image, it should not fallback to the example image.
Keeping this needs review in case that the approach needs review.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Only test fails are a common random failer. Will click retry and it may be green by the time anyone sees it.
- 🇺🇸United States bnjmnm Ann Arbor, MI
But I found a bug when testing with a JS component that has both the image prop and a text prop. Here in this GIF, I removed the media image, but then the text prop content also disappears from the preview, but it still exists in the form. And then when you type in the text prop, it does return to the preview.
I expanded the two e2e tests at the end of media-library.cy.js to include this scenario and they are passing. Perhaps the issue was resolved by a recent commit - many bugfixes recently.
- 🇬🇧United Kingdom longwave UK
I manually tested this and it works great but I'm a bit uneasy about the
isRemovedProp()
method as well - I think we should just be able to drop the values from resolved and source entirely and let the server side figure out that we need to go back to the default? - 🇺🇸United States bnjmnm Ann Arbor, MI
Good news
It looks like changes in 0.x that happened since I came up with the solution ~3 months ago means there's way less backend hacky stuff needed.Bad news
Getting the e2e tests to pass reliably - despite the IRL version working fine - is not going smoothly at all. I'll just set this to needs work until I get that slop more agreeable. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Marked 🐛 Cannot remove media Active as a duplicate of this.