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

Autoconvert assets if browser supports it #18012

Merged
merged 19 commits into from Apr 12, 2023
Merged

Autoconvert assets if browser supports it #18012

merged 19 commits into from Apr 12, 2023

Conversation

Nitwel
Copy link
Member

@Nitwel Nitwel commented Mar 31, 2023

Fixes #16068
Fixes ENG-157
Ref: #15918

@Nitwel Nitwel requested a review from a team March 31, 2023 13:19
@Nitwel Nitwel self-assigned this Mar 31, 2023
@Nitwel Nitwel requested review from paescuj, licitdev and azrikahar and removed request for a team March 31, 2023 13:19
Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets update this to be a ?format=auto query param instead. I don’t think we need it to be an environment variable

@Nitwel
Copy link
Member Author

Nitwel commented Apr 12, 2023

Updated it to a query option.

The previous approach did not work because the format property was passed to sharp with the value "auto" and this is not a valid option
@paescuj paescuj marked this pull request as draft April 12, 2023 11:01
@paescuj
Copy link
Member

paescuj commented Apr 12, 2023

Added a quick & dirty (but so far successful) approach in 7922e1c to auto transform images within the app as per #16068 (comment).

@Nitwel Could you verify if the idea of allowing the format=auto option along with the key option makes sense and if so clean this pull request up so we can get it over the line?
Thanks! 😃

@Nitwel
Copy link
Member Author

Nitwel commented Apr 12, 2023

It doesn't make sense to allow for format=auto when having a key set, as a key references a preset that contains the formatting. Meaning, we should change the presets instead of hacking our way around it. 😄

But you brought up a good point which is that I did forgot a presets implementation for assets. Added that now.

@rijkvanzanten
Copy link
Member

@Nitwel @paescuj This is looking very promising so far! What's left TBD to get this review-ready / merged? 🙂

@Nitwel
Copy link
Member Author

Nitwel commented Apr 12, 2023

Only a quick lint run 😅 and some testing but I'm happy where it's at right now.

@paescuj paescuj marked this pull request as ready for review April 12, 2023 15:04
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Nitwel for giving it the finishing touch 😄
This is a great addition, especially from the app side 👍
Nice clean-up, all my tests succeeded!

@paescuj paescuj enabled auto-merge (squash) April 12, 2023 15:06
@paescuj paescuj merged commit 898b580 into main Apr 12, 2023
7 checks passed
@paescuj paescuj deleted the fix-16068 branch April 12, 2023 15:08
@rijkvanzanten rijkvanzanten added this to the Next Release milestone Apr 13, 2023
hanneskuettner pushed a commit to hanneskuettner/directus that referenced this pull request Apr 18, 2023
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
meditadvisors pushed a commit to ciso360ai/directus-mod that referenced this pull request May 13, 2023
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tif image conversion
3 participants