rappasoft / laravel-livewire-tables

A dynamic table component for Laravel Livewire
https://rappasoft.com/docs/laravel-livewire-tables/v2/introduction
MIT License
1.7k stars 320 forks source link

[Bug]: src attribute empty for ImageColumn column #1601

Closed nathan-io closed 6 months ago

nathan-io commented 6 months ago

What happened?

When using ImageColumn, the src attribute is empty in the rendered HTML. The alt attribute is being set as expected.

            ImageColumn::make('Image', 'default_image')
                ->location(fn($row) => $row->default_image)
                ->attributes(fn($row) => [
                    'alt' => $row->name,
                ]),

An example of what the above renders on a row:

<td wire:key="table-table-td-136-image" style="" class="px-6 py-4 whitespace-nowrap text-sm font-medium dark:text-white">
        <img src="" alt="Some Product Name">
</td>

Note that defaultImage() is an accessor on our model, and we have default_image in $appends.

However, that doesn't seem to make a difference. It also doesn't seem to make no difference if I remove the second argument to make().

If I load that particular model in Tinker, here is the value returned by default_image:

"https://azrstgp1.blob.core.windows.net/goldimages/50pes_obv_600x600_png"

We're including id in the query, and other accessors are working fine in other columns.

How to reproduce the bug

No response

Package Version

v3.1.5

PHP Version

None

Laravel Version

v10.31.0

Alpine Version

3.12.0

Theme

None

Notes

No response

Error Message

No response

lrljoe commented 6 months ago

Will review next week during a Columns review

Ensure you add the field in setAdditionalSelects in your configure if you're not using the field anywhere else, as the package only retrieves USED fields.

nathan-io commented 6 months ago

default_image isn't a column we can select. We have a defaultImage() custom attribute accessor on our model.

Custom attributes are working as expected in another columns where we're using label(), so perhaps incorrectly assumed that it would work in this case.

lrljoe commented 6 months ago

In that case, you just need to ensure any fields that it does use to generate that custom attribute are included in setAdditionalSelects

nathan-io commented 6 months ago

Thank you, this was precisely the issue!

The accessor needed a JSON column which I added in setAdditionalSelects. Afterward, the value for default_image is coming through and the images are displaying.

I'm running into a responsiveness issue when setting the image width, where images are a different size for rows on the first page than they are on subsequent pages. Not sure why, but this is an unrelated issue.

lrljoe commented 6 months ago

If you raise a separate thread for the other issue I'll take a look.

Is it just one json attribute that your accessor is returning?

If just the one, then you can cast it as json in your Model, and Laravel will make it available more cleanly, and you won't need to utilise the accessor approach repetitively.

I'm sure you're aware of this, but figured someone may stumble upon this and find it useful.

nathan-io commented 6 months ago

The accessor returns one value from images, which is a JSON column containing an array of strings. In our model images is cast as array.

nathan-io commented 6 months ago

One suggestion I would make is to somehow support conditional rendering of the <img> tag.

This would be useful in cases where, for example, not all products have an image. I'm currently handling this by displaying a placeholder:

->location(fn($row) => $row->default_image ?? Vite::image('placeholders/1x1.png')

However, it could be preferable for some use cases to just not render the <img> tag at all if the location value is empty.

lrljoe commented 6 months ago

Please can you do me a favour and raise a feature request for that, as it's a great idea and I don't want to lose track of it.

My initial thoughts is default behaviour is no img tag if null, and add a setPlaceholder() method to allow for placeholders to be used more easily.

I'm currently standardising some of the core methods, to make it easier to maintain the whole thing, but that'd seem like a quick enough win

You could of course use a mutator in your model if you want to just define a default throughout your app if it's missing, either as an interim or longer term, or use your default image path as the model default, depending on your usage/load But if a Feature Request is open for this then I'll make sure it gets done