spatie / statamic-responsive-images

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

v4 support #221

Closed ncla closed 1 year ago

ncla commented 1 year ago

Right now tests are failing due to entry creation (in GraphQL tests) which is slightly different from other entry creation in tests (can't recall right now why). This breaking change is documented in Statamic upgrade guide: https://statamic.dev/upgrade-guide/3-4-to-4-0#entry-date-behavior

Right now I am a bit busy and more likely to check this out more towards the week-end. If anyone can help out or know easy fix for this - feel free to chime in.

I am not sure if there are other issues yet besides this one.

bengs commented 1 year ago

I've submitted a PR to your fork which fixes the tests and one other v4 issue I encountered where the flex width of the breakpoint fields was nearly zero (removing a parent wrapper div in ResponsiveFieldtype.vue fixed that). The changes are working well with v4 except for one remaining issue where a selected breakpoint image is squashed / mostly unclickable but I think it's a Statamic core issue I've submitted a PR for it here: https://github.com/statamic/cms/pull/8112

ncla commented 1 year ago

Thank you, I had forgotten that I had added EntryFactory in this repository. I still need to spin this up on actual Statamic install and test it out, I will try to do this evening, and if all is good then a release will follow.

bengs commented 1 year ago

Sounds good 👍
The PR I submittted to core was closed because they need to keep the td cell outputted for multi-row asset situations (which I understand) - so there's still a remaining issue in this screenshot below where I've uploaded an image to the Default breakpoint but it's unclickable to edit (unless I delete it). My image does have an alt tag already set, but the empty <td class="w-24"></td> coming from Statamic Core's resources/js/components/fieldtypes/assets/AssetRow.vue component fills the space:

Screenshot 2023-05-11 at 17 13 30

Maybe some td:empty CSS scoped to the responsive image field type might work as a temporary fix, I'll get back to you.

ncla commented 1 year ago

Yeah, stuff looks kinda bad in narrow viewports. The "Browse" button also goes out of bounds. We are pushing the limits of the asset fieldtype. :sweat_smile:

bengs commented 1 year ago

Haha yes I noticed that too - I did experiment with making the first column 100% width just when all 4 columns are on display, but I decided against including it in the PR because I still like how you've done it all in one row per break point, it prevents the field taking up a lot of vertical space in bard.
I can confirm that some scoped css like this fixes the empty alt td problem, maybe included that with the plugin for now is worthwhile to make it useable with v4?

.responsive-fieldtype .asset-table-listing > table .asset-row > td:empty {
  display:none;
}

After pasting that rule in to my browser console I can then see/click to edit the image:

Screenshot 2023-05-11 at 17 45 02. I don't think the asset table will have more than one row in our use case so hiding the empty td should be ok to do?

ncla commented 1 year ago

Oh, I was thinking more full fledged, covering very narrow scenarios with CSS container queries. Both cases where the alt is needed and when it is already set.

bengs commented 1 year ago

Container queries could work well for dealing with overflowing breakpoint columns and yes might be best for the asset field too - my initial thoughts were the asset field could have similar width requirements whether the alt is or isn't set (you see 'set alt' where the filename would be, and show enough of the thumbnail on the left of it in both scenarios), but either way some CSS to handle / override core's asset-row layout will work because the empty fixed width td it outputs when the alt is set causes problems in the current layout

ncla commented 1 year ago

Not perfect but better than before. Container queries are awesome!

https://github.com/spatie/statamic-responsive-images/assets/5507083/bf3e338b-bc0d-491a-88d6-a0b05ee430e8