-
Notifications
You must be signed in to change notification settings - Fork 270
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
feat: module config .format
, last entry used for nuxt-image
#896
base: main
Are you sure you want to change the base?
Conversation
✅ Live Preview ready!
|
❌ Deploy Preview for nuxt-image failed.
|
@danielroe I've added one new unit test for the change but did not adapt existing tests - waiting for the solution to be discussed/confirmed. Would you have any thoughts on this? |
I think the first item in the list is to be preferred for 'modern' formats. I think the only reason we had special handling for the last item in |
yes and no, |
Maybe splitting the default format config between image <> picture would be a better choice? I have a feeling trying to cover all use cases at the cost of having hidden logic and edge case handling underneath could do more harm than good... |
The last item in the format array is not the legacy format though. I think if the user is specifying a default format then it makes sense to respect the first one for image as well. It's always overrideable on a per-image basis, and if fallback legacy image formats were important, then you can always use nuxt-picture. |
I'm good with this rationale.
Then I'll go ahead and complete this PR... |
That sounds good to me 👍 |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #896 +/- ##
=======================================
Coverage 89.75% 89.75%
=======================================
Files 44 44
Lines 2879 2879
Branches 319 320 +1
=======================================
Hits 2584 2584
Misses 294 294
Partials 1 1
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main worry here is that this now applies webp
default format to all images without user opt-in. For providers that support it, I think auto
should be the default, and it should fall back to the default value provided.
So maybe we need to remove the default of webp
and apply that as a default value for <nuxt-picture>
only.
Co-authored-by: Daniel Roe <[email protected]>
Co-authored-by: Daniel Roe <[email protected]>
Yes, that's what's been worrying me as well. I just noticed there's an incomplete point (2) in my initial message... Or would you re-consider
|
It's a good point. Maybe support global |
Do we already have global config I can see
I find (B) confusing, (C) very confusing. I could live with (A), unsure but (D) might be OK? |
Description
WIth #880 a module level config option
format
was added which currently applies to<nuxt-picture>
only.This PR extends nuxt-image to use this config parameter as a default/fallback for
<nuxt-image>
.TBC
['webp']
all<nuxt-image>
are going to be webp format, is this OK? Or should we remove the default value?References
Solves/completes: #657
Tasks
confirm using the last element of the array value is the way to go-> use first itemformat: ['webp']
-> applies to many tests)['webp']
?