-
Notifications
You must be signed in to change notification settings - Fork 273
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: auto detect image ratio #37
Conversation
Co-authored-by: Sébastien Chopin <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #37 +/- ##
===========================================
- Coverage 87.10% 62.50% -24.61%
===========================================
Files 21 3 -18
Lines 442 96 -346
Branches 103 26 -77
===========================================
- Hits 385 60 -325
+ Misses 57 36 -21
Continue to review full report at Codecov.
|
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.
Nice additions! Sorry for my verbose comments 🙈
const prefix = '/' | ||
|
||
return defu(providerOptions, { | ||
baseURL: `http://${defaultHost}:${defaultPort}${prefix}`, |
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.
- baseURL in browser may differ from
HOST
when deployed to production - For server-side, we may directly listen to a random port
In conclusion, it would be best if at least support two baseURL options and improve later
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 have created internalBaseURL
option for server-side usage. 4bd3543
(#37)
dir: options.dir | ||
}, | ||
{ | ||
name: 'remote', |
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.
We should somehow guard this to only enable during generate
to avoid service abuse
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.
We can deny all remote urls by default, and If the user wants to use a remote url she should add domains to module whitelist.
What do you think?
Co-authored-by: Sébastien Chopin <[email protected]> Co-authored-by: pooya parsa <[email protected]>
Using the feature introduced in unjs/ipx#8 we can detect image original width and height and calculate image ratio to prevent CLS and remove explicit image width.
Fix #30