search

spatie / statamic-responsive-images

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

Set URL absolute instead of relative #190

Closed SteJW closed 1 year ago

SteJW commented 1 year ago

Like mentioned in this issue https://github.com/spatie/statamic-responsive-images/issues/101 When using the addon headless, we need to add the FULL URL manually in the frontend. This way, we won't have to.

This will set the relative URL to an absolute URL.

ncla commented 1 year ago

Hi, thanks for opening a PR,

Tests are failing.

This change will be a breaking one, as anyone who prepended a domain before the relative path for this will need to update again their code after updating this addon. As such this should be preferably adjusted to something non-breaking.

I know you did not open the issue #101 but it says:

where the default asset fields respect the settings in filesystems.php the srcSet-fields don't.

Assets are different from Glide manipulated images, so the config that is for asset filesystem is not the same as for image manipulation. As such it is normal that image manipulations does not reuse those same configurations.

If you were to update assets.image_manipulation config to use a filesystem, then in theory you should get absolute URLs. https://statamic.dev/image-manipulation#caching Although the code URL::makeRelative($manipulator->build()); maybe turns it back into relative URL anyway.

It makes more sense to me personally to prepend the domain name in my app. Are there other concerns that I might be missing other than developer experience?

SteJW commented 1 year ago

Thanks for replying to the PR.

I haven't ran the Unit test, sorry about that.

The thing with this issue is that when you return a srcSet, you basically get a string that divides images and width settings by comma's. You would have to walk through the string to change it. We're also adding data to Algolia and this image is part of the search overview. That's why we wanted to add it there as well. Would've been nice if we control the correct url/path from Statamic.

The change will for sure require some changes if you're already prepending the URL. Maybe consider this to be a 3.x version?

ncla commented 1 year ago

Got it. This is more clearer now to me.

I think something like param or config with default being "relative" URL could work. If you need absolute URLs, you opt-in. Would prevent from this being breaking change.

SteJW commented 1 year ago

That's a good idea. I added a config setting for it. Could you review it?

ncla commented 1 year ago

Looks OK. Are you able to write tests for this?

ncla commented 1 year ago

Hope you do not mind taking a bit over, wrote some tests, and one failing test is for #191 I discovered while testing this PR.

ncla commented 1 year ago

Thanks!

SteJW commented 1 year ago

Thanks @ncla! I've been a bit busy last few days so I was unable to write the test. But glad it worked out!