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(common): Warn on fill ngOptimizedImage without height #48036
Conversation
Add a warning if a fill-mode image is rendered without height. This is a common occurence if the user doesn't properly set the 'position' attribute of the parent element.
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.
LGTM
dir: NgOptimizedImage, img: HTMLImageElement, renderer: Renderer2) { | ||
const removeListenerFn = renderer.listen(img, 'load', () => { | ||
removeListenerFn(); | ||
const renderedHeight = parseFloat(img.clientHeight as any); |
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.
Nit: can we replace any
with a more specific type? e.g. string|null
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.
more importantly: why do we need parseFloat
? Seems like the type of HTMLImageElement:clientHeight
is a number already: https://www.typescriptlang.org/play?#code/MYewdgzgLgBAlgWwOYwLwzAVwDbZgQwgLAE8CiAJAFQFkAZASQXyQFMBRbVhVsKAbgBQg0JFgALNPGQA6YNji8oFVnCTiBwgA74AThFYAxbCHxQAFOICU-IA
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.
Yeah, I think the parseFloat pattern got pasted around in a few places where it's not needed. I just added a cleanup task to the tracker to go back through and review typing and usages of parseFloat().
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.
Thanks for the review Pawel! Since Alex added a cleanup item to the tracking doc, I'm moving this PR to the merge queue, so we include the fix into the v15 release.
…edImage directive This commit adds extra e2e tests for the fill mode checks in the NgOptimizedImage directive to make sure a warning is logged in a console.
LGTM but left a cleanup comment. |
LGTM! Thanks for your help with the e2e tests |
This PR was merged into the repository by commit d6dc6b6. |
Add a warning if a fill-mode image is rendered without height. This is a common occurence if the user doesn't properly set the 'position' attribute of the parent element. PR Close #48036
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
) Add a warning if a fill-mode image is rendered without height. This is a common occurence if the user doesn't properly set the 'position' attribute of the parent element. PR Close angular#48036
…edImage directive (angular#48036) This commit adds extra e2e tests for the fill mode checks in the NgOptimizedImage directive to make sure a warning is logged in a console. PR Close angular#48036
This PR includes one final fix for the ngOptimizedImage directive. Currently it does not warn if the image is in fill mode and the rendered height of the image is 0. This is a fairly common failure case when starting out with the image directive, which is caused by not properly setting the
position
style of the parent element.The warning added in this PR will help new users diagnose why their fill image is not appearing on the page.
Note: I am currently unable to run (and therefor unable to author) e2e tests on my machine, and this feature can only be tested in an e2e context. @AndrewKushnir, @pkozlowski-opensource or @kara, could one of you help me get an e2e test added for this change? It would follow the basic pattern used in
packages/core/test/bundling/image-directive/e2e/image-distortion/image-distortion.e2e-spec.ts