-
Notifications
You must be signed in to change notification settings - Fork 693
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 image svg #2727
base: master
Are you sure you want to change the base?
Fix image svg #2727
Conversation
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 its good to add a demo item to the webDemo app
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 can you create a version, so we can test it before the merge ? in our Editor
@AmitShwarts, can you give it a try once, @mendyEdri shares a version ?
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.
7.8.0-snapshot.3463
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.
@mendyEdri as we just talked, we're getting lot's of issues of width, height and resizeMode.
all of the page layout breaks for some reason because of this.. not sure why
src/components/image/index.tsx
Outdated
useImageInsideContainer && styles.shrink | ||
]; | ||
|
||
if (!this.shouldUseImageBackground() && Constants.isWeb) { |
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.
Constants.isWeb
will be always false, as you have index.web.tsx
src/components/image/index.web.tsx
Outdated
<img | ||
{...others} | ||
src={finalSource} | ||
style={StyleSheet.flatten([imageViewStyle, styles.containImage]) as CSSProperties} |
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.
why do we need to always flat this ?
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.
Same as you guys did, thought it's for flattening any sort of imageProps passed etc
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'm not sure we need it here, on the Icon comp it was because the button passes some default styles as array.
not sure its the case here.
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 added some comments, but... I have to say that I don't like this solution, it'll require Uilib to maintain two image files, and I'm confident that it will be forgotten.
react-native-web supports RN's Image, so we should re-check what doesn't work and close the gaps.
Until now we added the .web files only when something isn't supported on the web and I think we should keep doing it instead of duplicating to code.
@@ -119,7 +119,7 @@ | |||
"react-test-renderer": "18.2.0", | |||
"reassure": "^0.4.1", | |||
"shell-utils": "^1.0.10", | |||
"typescript": "4.9.5" | |||
"typescript": "^4.9.5" |
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.
Is it part of this PR in purpose?
isGif() { | ||
if (Constants.isAndroid) { | ||
const {source} = this.props; | ||
const url = _.get(source, 'uri'); | ||
const isGif = /(http(s?):)([/|.|\w|\s|-])*\.gif/.test(url ?? ''); | ||
return isGif; | ||
} | ||
} |
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 believe we can remove it here
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.
Something looks different in the webDemo project in the Animated Image
example, when I run it with the changes from this PR.
|
||
return ( | ||
// @ts-expect-error | ||
<View style={imageViewStyle}> |
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.
Why do you add this extra View
and not pass the style to the img
component?
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 guess because of the overlay, but it seems that both have the same imageViewStyle
which is wired a bit
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.
@OrenZak is right
I don't mind. atm in production sources with type of string (either source={url} or images actual data) are rendering the SVG component which causes issues and left us with production bugs. Also, the current mobile image implementation cannot render ImageBackground properly in web. I suggest to assign someone from the team to address it :) |
@ethanshar can you help ? |
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 we want to go with a duplicate file for the Image component just for web.
These type of solutions won't scale, we can't maintain two identical files for two platforms..
Either we come up with a better solution, or please open a detailed ticket and we'll tailor this properly.
Is this PR still relevant? can we close it? |
It is. |
But we don't use this fix, it's not merged. |
We do, we use this version in web-editor. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
Until now, all images with url used
SvgImage
component, because source was a url andisSvg
returnedtrue
.Changelog
Added support for
Image
webAdditional info