-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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
refactor(common): allow string urls in NgOptimizedImage
input
#54901
base: main
Are you sure you want to change the base?
Conversation
0969c97
to
96ab532
Compare
96ab532
to
d7f8d2b
Compare
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.
Didn't we talk about throwing an error if the placeholder dimensions are too large? Concerned that we will allow folks to unintentionally add large placeholder images
I don't remember such a discussion, are we talking about pixel dimension or the image size (in kB) ? Either way, here are my thoughts:
|
@JeanMeche It's a requirement in the design doc (pinged you in the doc so you can see it). We didn't talk about it in any great detail, but the idea was to sanity check the dimensions (width/height). While it's true that dimensions != intrinsic size, we can still do a basic check. A placeholder that is 1000 x 1000 is egregious even if it's a simple image. |
With this commit it is now possible to pass an url as placeholder for the image directive.
d7f8d2b
to
6f8a7a6
Compare
@@ -117,6 +117,11 @@ const FIXED_SRCSET_HEIGHT_LIMIT = 1080; | |||
*/ | |||
export const PLACEHOLDER_BLUR_AMOUNT = 15; | |||
|
|||
/** | |||
* Placeholder dimension (height or width) limit to warn when the crossed. |
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.
* Placeholder dimension (height or width) limit to warn when the crossed. | |
* Placeholder dimension (height or width) limit in pixels. Angular produces a warning | |
* when this limit is crossed. |
* Warns if placeholder's dimension are over a threshold. | ||
*/ | ||
function assertPlaceholderDimensions(dir: NgOptimizedImage, width: number, height: number) { | ||
if (width > PLACEHOLDER_DIMENSION_LIMIT || height > PLACEHOLDER_DIMENSION_LIMIT) { |
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 understanding is that we want to check the size of a real placeholder image, not the width
and height
attribute values. @kara is that correct?
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.
reviewed-for: fw-common, public-api
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.
reviewed-for: public-api
With this commit it is now possible to pass an url as placeholder for the image directive.