-
Notifications
You must be signed in to change notification settings - Fork 734
Image - pass width and height to image style #2384
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
Conversation
The example below seems to be working fine on master
Can you please provide an example that fails? |
This example doesn't work on master... |
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.
Please add the relevant example and a
src/components/image/index.tsx
Outdated
@@ -233,6 +236,7 @@ class Image extends PureComponent<Props, State> { | |||
aspectRatio && {aspectRatio}, | |||
!useImageInsideContainer && margins, | |||
useImageInsideContainer && styles.containImage, | |||
!cover && sizeProps && {width, height}, |
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 don't think the cover
is needed here, if the user makes a mistake they can fix it (this will be bugged without an overlay as well)
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.
Also, personally I prefer to remove the extra const sizeProps
.
Maybe just {width, height}
or (width || height) && {width, height}
?
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.
What do you mean by 'this will be bugged without an overlay as well'? This solution works whether you pass overlay or not...
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 I meant that if the user adds width and forgets height (or vice versa, or adds a width with cover etc) they will have bugs that they will need to fix.
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.
Yes, that's for sure
// style={!cover && {width: DEFAULT_SIZE, height: DEFAULT_SIZE}} | ||
width={!cover ? DEFAULT_SIZE : undefined} | ||
height={!cover ? DEFAULT_SIZE : undefined} |
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.
Was this on purpose?
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.
Yes, I remove the 'style' prop
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.
Added a small comment, approved
Description
When passing 'width' and 'height' the image doesn't render (only when they are passed in style).
Image - pass width and height to image style.
Changelog