-
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
fix(cloudinary): fit types, auto format & quality #76
Conversation
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
===========================================
- Coverage 73.22% 49.64% -23.59%
===========================================
Files 21 4 -17
Lines 549 139 -410
Branches 130 34 -96
===========================================
- Hits 402 69 -333
+ Misses 145 70 -75
+ Partials 2 0 -2
Continue to review full report at Codecov.
|
export default <RuntimeProvider> { | ||
getImage (src: string, modifiers: ImageModifiers, options: any) { | ||
const operations = operationsGenerator(modifiers) | ||
const mergeModifiers = { |
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.
Can be modifiers = { ...defaultModofiers, ...modifiers }
😊
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.
Would love to do it. Actually I tried it before, the problem is in image.ts
, format
is passed with undefined
value
if (!size.format) {
size.format = operations.format
}
And here :
const { url } = image(src, {
...operations,
width: size.width,
format: size.format
})
so it always overwrite the default value with undefined
if there is none passed. Either we fix the code logic here, or we check per provider :). WDYT?
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.
I think defaults should be null
then we can use defu
here /cc @farnabaz
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.
As we are refactoring components, This can be fixed in the refactor process
Thank you @mayashavin ❤️ |
Add the following: