spatie / statamic-responsive-images

Responsive images for Statamic 3
MIT License
99 stars 29 forks source link

this.name undefined when adding a new set in the middle of a replicator field #186

Closed danielsmink closed 1 year ago

danielsmink commented 1 year ago

Bug description

We use the responsive field to create sliders. We do so using a replicator field with a set containing a responsive field and text field for a title. When adding in a slide in the middle of the set of images the slide underneath is cleared from it's data and a error is shown in the console (see screenshot)

Screenshot 2022-12-08 at 20 29 51

How to reproduce

Create a blueprint with a replicator field with the set containing the responsive field. You can add new sets and remove them without any problem, but if you add a set in the middle (in between two sets) an error will occur. This is probably due to the publish fields not exposing their correct name. They seem to be stuck on the previous array index.

Logs

No response

Environment

PHP 8.1.13
Laravel Framework 9.43.0
Statamic 3.3.60
Responsive images 2.14.4

Installation

Fresh statamic/statamic site via CLI

Antlers Parser

regex (default)

Additional details

No response

ncla commented 1 year ago

Is this the error you are referencing?

image

Just checking but does this impact functionality for you? Editing, saving works fine for me even with the error.

I was only able to reproduce this error in console when deleting a set.

danielsmink commented 1 year ago

That's the one @ncla it impacts functionality if you add a slide/set in between two others. In that case it removes the data from the follow up slide as well. As they keep referencing the old array index number.

After I click the plus in the scenario underneath the data in that set will be gone as well.

Screenshot 2022-12-08 at 22 56 37

As you can see in the screenshot I've put the name in for debugging purposes and you can see the fields referencing the old array index.

Screenshot 2022-12-08 at 23 00 08
el-schneider commented 1 year ago

I can confirm having the exact same error. All data after the inserted set get's erased.

Update: I created a minimal reproduction here: https://github.com/el-schneider/statamic-ri-deleted-sets

el-schneider commented 1 year ago

Maybe this got introduced with: https://github.com/statamic/cms/pull/7000

ncla commented 1 year ago

Possible. Can you try downgrading cms to version 3.3.51 which is one version before this PR to check if this PR introduced the bug? Thanks!

On Tue, Dec 13, 2022 at 10:51, el-schneider @.***> wrote:

Maybe this got introduced with: statamic/cms#7000

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

danielsmink commented 1 year ago

@ncla just tested with 3.3.51 same error occurs. The bug was reported to us by a client on October 27.

ncla commented 1 year ago

Just to give a small update: I can reproduce this issue on my end too and see everything you folks have described.

It's something to do with the publish container indeed, as this addons fieldtype wraps multiple core fieldtypes into one in publish container, which seems to be the issue from todays small research.

There might be perhaps an even older CMS version than the previously mentioned one that might work.

As a temporary workaround until a proper fix is found: if you are just using the default breakpoint from the Responsive fieldtype, you can just use Assets fieldtype, and pass that asset value to the src parameter for responsive tag.

el-schneider commented 1 year ago

@ncla Thank you for looking into this!

My temporary workaround is to hide the in-between buttons, with some CSS 🤡

danielsmink commented 1 year ago

Thanks for the workaround @ncla that works for us!

el-schneider commented 1 year ago

@ncla Thank you for looking into this!

My temporary workaround is to hide the in-between buttons, with some CSS 🤡

Just realized that it still allows my clients to duplicate or delete certain sets and wipe all following ones, so it's not really a great solution. Since I'm relying on breakpoints the other workaround also doesn't help me.

el-schneider commented 1 year ago

For anyone looking for a solution and as the responsive tag also lets you choose different fields per breakpoint I ended up doing this, using native asset fields and having a nice fallback if one stays empty:

{{ base_image = image or image_desktop }}
{{ responsive:base_image :desktop:src="image_desktop"
ncla commented 1 year ago

Hey @el-schneider @danielsmink I have pushed a new release v2.15.1 that includes fix for this. Can you click around and see if it is fixed for you, and that there are no regressions?

danielsmink commented 1 year ago

Hi @ncla it works as expected! Great. Thanks so much.