Skip to content

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

Merged
merged 9 commits into from
Jan 9, 2023

Conversation

Inbal-Tish
Copy link
Collaborator

@Inbal-Tish Inbal-Tish commented Dec 19, 2022

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

⚠️ Image - pass 'width' and 'height' to image style to fix layout props not applied to Image without 'style'.

@Inbal-Tish Inbal-Tish reopened this Dec 19, 2022
@M-i-k-e-l
Copy link
Collaborator

The example below seems to be working fine on master

<Image
  source={{uri: 'https://static.wixstatic.com/media/d911269bdf7972c9a59ba30440cb3789.jpg_128'}}
  width={50}
  height={50}
/>

Can you please provide an example that fails?

@Inbal-Tish
Copy link
Collaborator Author

The example below seems to be working fine on master

<Image
  source={{uri: 'https://static.wixstatic.com/media/d911269bdf7972c9a59ba30440cb3789.jpg_128'}}
  width={50}
  height={50}
/>

Can you please provide an example that fails?

This example doesn't work on master...

Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a 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 ⚠️

@@ -233,6 +236,7 @@ class Image extends PureComponent<Props, State> {
aspectRatio && {aspectRatio},
!useImageInsideContainer && margins,
useImageInsideContainer && styles.containImage,
!cover && sizeProps && {width, height},
Copy link
Collaborator

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)

Copy link
Collaborator

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}?

Copy link
Collaborator Author

@Inbal-Tish Inbal-Tish Jan 8, 2023

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...

Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Comment on lines +124 to +126
// style={!cover && {width: DEFAULT_SIZE, height: DEFAULT_SIZE}}
width={!cover ? DEFAULT_SIZE : undefined}
height={!cover ? DEFAULT_SIZE : undefined}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this on purpose?

Copy link
Collaborator Author

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

Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a 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

@Inbal-Tish Inbal-Tish merged commit 5d75614 into master Jan 9, 2023
@Inbal-Tish Inbal-Tish deleted the fix/Image_mandatory_style branch January 11, 2023 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants