Fix image svg #2727

mendyEdri · 2023-08-28T14:30:43Z

Description

Until now, all images with url used SvgImage component, because source was a url and isSvg returned true.

Changelog

Added support for Image web

Additional info

OrenZak · 2023-09-04T06:22:36Z

package.json

I think its good to add a demo item to the webDemo app

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 ?

7.8.0-snapshot.3463

@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

OrenZak · 2023-09-04T06:25:34Z

src/components/image/index.tsx

+      useImageInsideContainer && styles.shrink
+    ];
+
+    if (!this.shouldUseImageBackground() && Constants.isWeb) {


Constants.isWebwill be always false, as you have index.web.tsx

OrenZak · 2023-09-04T06:29:51Z

src/components/image/index.web.tsx

+        <img 
+          {...others} 
+          src={finalSource} 
+          style={StyleSheet.flatten([imageViewStyle, styles.containImage]) as CSSProperties}


why do we need to always flat this ?

Same as you guys did, thought it's for flattening any sort of imageProps passed etc

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.

lidord-wix

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.

lidord-wix · 2023-09-04T15:54:55Z

package.json

@@ -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"


Is it part of this PR in purpose?

src/components/image/index.web.tsx

lidord-wix · 2023-09-04T16:31:13Z

src/components/image/index.web.tsx

+  isGif() {
+    if (Constants.isAndroid) {
+      const {source} = this.props;
+      const url = _.get(source, 'uri');
+      const isGif = /(http(s?):)([/|.|\w|\s|-])*\.gif/.test(url ?? '');
+      return isGif;
+    }
+  }


I believe we can remove it here

lidord-wix · 2023-09-04T16:41:21Z

src/components/image/index.web.tsx

Something looks different in the webDemo project in the Animated Image example, when I run it with the changes from this PR.

lidord-wix · 2023-09-04T16:45:40Z

src/components/image/index.web.tsx

+
+    return (
+      // @ts-expect-error
+      <View style={imageViewStyle}>


Why do you add this extra View and not pass the style to the img component?

i guess because of the overlay, but it seems that both have the same imageViewStyle which is wired a bit

@OrenZak is right

mendyEdri · 2023-09-07T05:29:07Z

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.

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.
moreover, using the mobile image, rendered styling weird, and wasn't affecting by image-ratio prop.

I suggest to assign someone from the team to address it :)
thanks!

OrenZak · 2023-09-07T07:05:53Z

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.

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. moreover, using the mobile image, rendered styling weird, and wasn't affecting by image-ratio prop.

I suggest to assign someone from the team to address it :) thanks!

@ethanshar can you help ?

ethanshar

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.

ethanshar · 2023-11-09T06:57:33Z

Is this PR still relevant? can we close it?

mendyEdri · 2023-11-09T06:58:34Z

Is this PR still relevant? can we close it?

It is.
Removing it, still breaking images in web-editor

ethanshar · 2023-11-12T05:50:26Z

But we don't use this fix, it's not merged.
Is there still an issue with SVG?

mendyEdri · 2023-11-12T07:03:02Z

But we don't use this fix, it's not merged.

Is there still an issue with SVG?

We do, we use this version in web-editor.
And yes, there is still an issue

stale · 2024-03-13T10:58:28Z

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.

mendyEdri and others added 9 commits August 28, 2023 17:30

fix image svg

5ecca10

check for svg closing tag instead of checking only for string type

7c7b0f5

lint

d1ed4b2

Fix TS errors

c2ec347

check for svg data as well

d51c723

ts

e8e91ff

support both svg and image

9d6d5ca

lint

307f60d

lint

750eba0

mendyEdri requested a review from lidord-wix August 30, 2023 09:22

mendyEdri added 4 commits August 30, 2023 12:28

split web and mobile

655173e

overlay

690828d

dynamic aspect-ratio

6847a10

image fit

1ba6557

mendyEdri changed the title ~~fix image svg~~ Fix image svg Aug 30, 2023

mendyEdri requested review from ethanshar and OrenZak August 30, 2023 18:00

OrenZak reviewed Sep 4, 2023

View reviewed changes

src/components/image/index.tsx Show resolved Hide resolved

OrenZak reviewed Sep 4, 2023

View reviewed changes

remove old web implementation

124b7a4

lidord-wix reviewed Sep 4, 2023

View reviewed changes

ethanshar reviewed Sep 13, 2023

View reviewed changes

Merge branch 'master' into maybe-fix-assets

25e4214

mendyEdri added 8 commits November 22, 2023 18:53

Merge branch 'master' into maybe-fix-assets

7428120

remove default alt image

26610c2

Merge branch 'master' into maybe-fix-assets

192201c

revert lint

8336244

remove default alt

898f7c8

handle missing asset - alt

efd62a3

remove lint autofix

3ff826b

remove unsued

4c7b3cb

stale bot added the wontfix label Mar 13, 2024

fix conflicts

e9b3367

stale bot removed the wontfix label Apr 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix image svg #2727

Fix image svg #2727

mendyEdri commented Aug 28, 2023 •

edited

OrenZak Sep 4, 2023

OrenZak Sep 4, 2023

mendyEdri Sep 4, 2023

AmitShwarts Sep 5, 2023

OrenZak Sep 4, 2023 •

edited

OrenZak Sep 4, 2023

mendyEdri Sep 4, 2023

OrenZak Sep 4, 2023

lidord-wix left a comment

lidord-wix Sep 4, 2023

lidord-wix Sep 4, 2023

lidord-wix Sep 4, 2023

lidord-wix Sep 4, 2023

OrenZak Sep 4, 2023

mendyEdri Sep 5, 2023

mendyEdri commented Sep 7, 2023

OrenZak commented Sep 7, 2023

ethanshar left a comment

ethanshar commented Nov 9, 2023

mendyEdri commented Nov 9, 2023

ethanshar commented Nov 12, 2023

mendyEdri commented Nov 12, 2023

stale bot commented Mar 13, 2024

Fix image svg #2727

Are you sure you want to change the base?

Fix image svg #2727

Conversation

mendyEdri commented Aug 28, 2023 • edited

Description

Changelog

Additional info

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OrenZak Sep 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lidord-wix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mendyEdri commented Sep 7, 2023

OrenZak commented Sep 7, 2023

ethanshar left a comment

Choose a reason for hiding this comment

ethanshar commented Nov 9, 2023

mendyEdri commented Nov 9, 2023

ethanshar commented Nov 12, 2023

mendyEdri commented Nov 12, 2023

stale bot commented Mar 13, 2024

mendyEdri commented Aug 28, 2023 •

edited

OrenZak Sep 4, 2023 •

edited