Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'toInlineResponse' to Media #2636

Merged
merged 4 commits into from
Nov 17, 2021

Conversation

mbardelmeijer
Copy link
Contributor

Follow-up of #1358

@freekmurze
Copy link
Member

Thanks! Could you add a test for this and also update the docs?

@mbardelmeijer
Copy link
Contributor Author

Added!

@rcerljenko
Copy link
Contributor

@mbardelmeijer @freekmurze would be great if you can optionally pass a conversion name to these functions. eg:

return $media->toResponse($request, 'thumb');

and how about responsive images?

@freekmurze
Copy link
Member

would be great if you can optionally pass a conversion name to these function

Good idea!, could you take care of this @mbardelmeijer

Responsive images are out of scope for now.

@mbardelmeijer
Copy link
Contributor Author

@freekmurze @rcerljenko I've checked if it would be possible to add conversion support to these methods, but ran into that the stream method from the Filesystem doesn't support passing a conversion name.

We can get the file via the default methods, but then we don't have the memory optimized stream function to use.

We can add a 'getConversionStream' method for instance in the Filesystem, and a conversionStream to the Media object, but I do think this would grow the scope of the PR too much. Thoughts?

@rcerljenko
Copy link
Contributor

@mbardelmeijer yes it adds more work.. let @freekmurze decides

@freekmurze freekmurze merged commit 71ce715 into spatie:main Nov 17, 2021
@freekmurze
Copy link
Member

Let's keep it simple for now.

Thanks @mbardelmeijer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants