Expo image #4

WojtekBoman · 2023-10-16T08:00:09Z

Details

This PR improves the way of displaying images on mobile platforms. The most relevant changes are the removal of react-native-svg-transformer and the use expo-image to display images in the app. Previously all icons in .svg format were imported as a React components, what in result has negative influence on app's performance. Thanks to the usage of expo-image, we no longer need to transform svg icons and can use them directly by passing them to the source property of the Image component from 'expo-image' library.

Fixed Issues

Reduced number of rendered elements in the component tree and better rendering times.

Rendering SignInPage with react-native-svg on Android

Rendering SignInPage with expo-image on Android

There are not issues associated with this PR.
$
PROPOSAL:
Original Problem/Solution was posted here: https://expensify.slack.com/archives/C01GTK53T8Q/p1695892593681869

Tests

Go through the application and check for any problems with displaying images

Verify that no errors appear in the JS console

Offline tests

QA Steps

Go through the application and check for any problems with displaying images

Verify that no errors appear in the JS console

PR Author Checklist

Screenshots/Videos

Android: Native

Screen.Recording.2023-11-03.at.16.06.14.mov

Android: mWeb Chrome

Screen.Recording.2023-11-06.at.11.29.16.mov

iOS: Native

Screen.Recording.2023-11-03.at.15.23.51.mov

iOS: mWeb Safari

Screen.Recording.2023-11-03.at.15.37.54.mov

MacOS: Chrome / Safari

Screen.Recording.2023-11-03.at.15.05.11.mov

MacOS: Desktop

Screen.Recording.2023-11-03.at.14.58.11.mov

mczernek · 2023-10-17T13:31:31Z

src/components/Icon/BankIcons.ts

+};
+
+type AssetIcon = {
+    icon: React.FC<SvgProps>;


We're using type from react-native-svg even though we're not really using react-native-svg here, am I right?

If so, let's try to find some other type here.

mczernek · 2023-10-17T13:43:14Z

src/components/Icon/index.native.js

+    contentFit: PropTypes.string,
+
+    /** Is icon displayed in its own color */
+    displayInDefaultIconColor: PropTypes.bool,


It is confusing for me, that there is a prop available in native only. I'd honestly remove it and make behavior identical to what happens on web - make fill color from theme as default and override it with whatever passed as fill prop, if passed.

But If I want to override it I have to pass null value through the fill property. Is it ok? I've added this property to make code more readable. When we want to display icon in original color, component should not have defined tintColor property or this property's value must be null. Instead of passing null through the fill property I created this prop.

Is there any place where we want to display Icon's color defined in SVG file without any tinting (either with color from theme or passed as property)? If so, why does it work on web without this additional boolean? If no, I think that passing undefined as fill is fine

Fill prop behaviour is different when we use expo-image because in this case we set tintColor for whole image, null value allows to use original icon color. When we pass this prop to component transformed by svg-transformer we can set fill prop's value directly in svg asset, it doesn't set this color for whole icon but only for the part which should be displayed in this color. It can be noticed on logo asset which i modified in this PR. I set fill prop's value directly in svg asset and it changed only the logo part of this asset not whole image, small icon which shows environment didn't change

mczernek · 2023-10-17T13:48:31Z

src/components/Icon/index.native.js

+    name: PropTypes.string,
+
+    /** Determines how the image should be resized to fit its container */
+    contentFit: PropTypes.string,


Why is that necessary on native and not on web?

On web we still use @svgr/webpack to transform svg icons to components, icons displayed as a components don't need to be additionally styled

mczernek · 2023-10-17T13:50:34Z

src/components/Icon/index.native.js

+                    style={[StyleUtils.getWidthAndHeightStyle(width, height), styles.bgTransparent, styles.overflowVisible]}
+                >
+                    <View style={iconStyles}>
+                        <Image


It seems that we could extract this component (and IconComponent in index.js) to two seperate files (one with native.js extension) and keep divergence between platforms at its minimum.

mczernek · 2023-10-17T14:01:46Z

src/components/AvatarCropModal/AvatarCropModal.js

@@ -44,7 +45,7 @@ const propTypes = {
    isVisible: PropTypes.bool.isRequired,

    /** Image crop vector mask */
-    maskImage: PropTypes.func,
+    maskImage: PropTypes.oneOfType([PropTypes.func, imagePropTypes.source]),


Seems that this exact type is used in several places to indicate a field that accepts image component - maybe we could define this type within imagePropTypes and reuse it?

mczernek · 2023-10-17T14:34:53Z

src/components/CardPreview/index.native.js

+
+    return (
+        <View style={styles.walletCard}>
+            <Image


Same as in other places, even though only this particular component differs between native and web, we have two big files that are mostly identical :(

Maybe we could create some component called <Image> of our own and have two implementations of it depending on platform?

mczernek · 2023-10-17T14:41:10Z

src/components/Image/index.native.js

-                    props.onLoad(evt);
-                }
-            }}
+            // onLoad={(evt) => {


Remove usuned code

mczernek · 2023-10-17T14:43:24Z

src/components/QRShare/index.native.js

+                onLayout={this.onLayout}
+            >
+                <View style={styles.expensifyQrLogo}>
+                    <Image


ANother place with two almost identical files.

mczernek · 2023-10-17T14:44:34Z

src/components/ValidateCode/JustSignedInModal.js

                        width={variables.modalTopIconWidth}
                        height={props.is2FARequired ? variables.modalTopIconHeight : variables.modalTopBigIconHeight}
                        src={props.is2FARequired ? Illustrations.SafeBlue : Illustrations.Abracadabra}
+                        name={props.is2FARequired ? 'Safe' : 'Abracadabra'}


Is Abracadabra intentional? :D

mczernek · 2023-10-17T14:46:41Z

src/components/MentionSuggestions.js

@@ -77,7 +76,7 @@ function MentionSuggestions(props) {
                        size={isIcon ? CONST.AVATAR_SIZE.MENTION_ICON : CONST.AVATAR_SIZE.SMALLER}
                        name={item.icons[0].name}
                        type={item.icons[0].type}
-                        fill={themeColors.success}
+                        // fill={themeColors.success}


Unused code

mczernek · 2023-10-17T14:47:16Z

src/components/MultipleAvatars.js

@@ -137,7 +138,7 @@ function MultipleAvatars(props) {
                    <Avatar
                        source={props.icons[0].source}
                        size={props.size}
-                        fill={themeColors.iconSuccessFill}
+                        // fill={themeColors.iconSuccessFill}


Unused code

mczernek · 2023-10-17T14:47:55Z

src/components/RoomHeaderAvatars.js

@@ -45,7 +44,7 @@ function RoomHeaderAvatars(props) {
                        <Avatar
                            source={props.icons[0].source}
                            imageStyles={[styles.avatarLarge]}
-                            fill={themeColors.iconSuccessFill}
+                            // fill={themeColors.iconSuccessFill}


Unused code - 2 places in this file

mczernek · 2023-10-17T14:48:11Z

src/components/SubscriptAvatar.js

                        size={isSmall ? CONST.AVATAR_SIZE.SMALL_SUBSCRIPT : CONST.AVATAR_SIZE.SUBSCRIPT}
-                        fill={themeColors.iconSuccessFill}
+                        // fill={themeColors.iconSuccessFill}


Unused code

mczernek · 2023-10-17T15:41:29Z

src/pages/ErrorPage/GenericErrorPage/index.js

@@ -33,6 +33,7 @@ function GenericErrorPage({translate}) {
                            <View style={styles.mb5}>
                                <Icon
                                    src={Expensicons.Bug}
+                                    name="Bug"


Very similar story again

mczernek · 2023-10-17T15:43:23Z

src/pages/iou/ReceiptDropUI/index.native.js

+        <DragAndDropConsumer onDrop={onDrop}>
+            <View style={[styles.receiptDropOverlay, styles.w100, styles.h100, styles.justifyContentCenter, styles.alignItemsCenter]}>
+                <View style={styles.receiptImageWrapper(receiptImageTopPosition)}>
+                    <Image


SAme story - almost identical files

mczernek · 2023-10-17T15:45:59Z

src/pages/settings/AboutPage/index.native.js

+                        <View style={[styles.flex1]}>
+                            <View style={styles.pageWrapper}>
+                                <View style={[styles.settingsPageBody, styles.mb6, styles.alignItemsCenter]}>
+                                    <Image


Once again, this is the only difference between this and index.js

mczernek · 2023-10-17T15:53:24Z

tests/unit/SidebarOrderTest.js

@@ -182,7 +182,7 @@ describe('Sidebar', () => {
                    // Then there should be a pencil icon and report one should be the first one because putting a draft on the active report should change its location
                    // in the ordered list
                    .then(() => {
-                        const pencilIcon = screen.queryAllByTestId('Pencil Icon');
+                        const pencilIcon = screen.queryAllByTestId('SvgPencil Icon');


Are those the only two test files that uses images test ids?

Signed-off-by: Ashutosh Khanduala <[email protected]>

Fix/ioutest

WojtekBoman force-pushed the expo-image branch 3 times, most recently from af156f6 to ba026f7 Compare October 17, 2023 08:57

mczernek reviewed Oct 17, 2023

View reviewed changes

src/components/Image/index.native.js Outdated

props.onLoad(evt);

}

}}

// onLoad={(evt) => {

Copy link

mczernek Oct 17, 2023

Choose a reason for hiding this comment

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

Remove usuned code

mczernek reviewed Oct 17, 2023

View reviewed changes

BartoszGrajdek pushed a commit that referenced this pull request Oct 18, 2023

progress #4: emojiList -> emojiListRef

7959d01

Signed-off-by: Ashutosh Khanduala <[email protected]>

WojtekBoman force-pushed the expo-image branch 2 times, most recently from a9339c6 to 3f2f41b Compare October 24, 2023 09:38

mczernek and others added 7 commits November 2, 2023 08:15

Add expo-modules to application.

30594c2

Update package-lock.json after adding expo-modules.

199d5d6

Bump kotlin to 1.8.20 to make it compatible with expo-modules.

87caddc

Prettier on metro.config.js

e38d43d

Add patch for plaid-sdk

4a40e3f

Display icons with expo-image

ca47d62

Fix displaying svg icons with own fill colors, fix prop types for icons

49f341a

WojtekBoman and others added 28 commits December 11, 2023 08:38

Remove expo-asset and unnecessary comments

9647214

Migrate ImageSVG to TS

95e49d0

Merge branch 'main' into expo-image

a8d3096

Fix ts in HeaderUtils

4d7b90c

Adjust home background fill color

17f0b67

Merge branch 'main' into expo-image

a4ec7dc

Merge branch 'main' into expo-image

221fe4e

Merge branch 'main' into expo-image

c483c8a

Fix imports order

4d78a56

Merge branch 'main' into expo-image

45c8e31

Merge branch 'main' into expo-image

4900127

Merge branch 'main' into expo-image

f8448ca

Replace themeColors with useTheme

5574239

Adjust new components to expo-image

57d57ec

Remove assets registry

ba157c1

Merge branch 'main' into expo-image

c939df1

Merge branch 'main' into expo-image

842830f

Merge branch 'main' into expo-image

09a703d

Refactor BackgroundImage, remove unnecessary props

1da3052

Merge branch 'main' into expo-image

fabb0b0

Merge branch 'main' into expo-image

b739e34

Add contentFit prop to MenuItem

dcc4994

Merge branch 'main' into expo-image

b856010

Merge branch 'main' into expo-image

06638b2

chore: merge current main

0deaafd

Refactor BankIcons nad PaymentMethodList

3dc4373

Run prettier on BankIconsUtils

e55ca4d

Merge branch 'main' into expo-image

7c4ef3f

BartoszGrajdek merged commit 9c46375 into main Dec 22, 2023
11 of 14 checks passed

cdOut pushed a commit that referenced this pull request Sep 8, 2024

Merge pull request #4 from ishpaul777/fix/ioutest

49b79ca

Fix/ioutest

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expo image #4

Expo image #4

WojtekBoman commented Oct 16, 2023 •

edited

Loading

mczernek Oct 17, 2023

mczernek Oct 17, 2023

WojtekBoman Oct 18, 2023

mczernek Oct 18, 2023

WojtekBoman Oct 18, 2023 •

edited

Loading

mczernek Oct 17, 2023

WojtekBoman Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

mczernek Oct 17, 2023

Expo image #4

Expo image #4

Conversation

WojtekBoman commented Oct 16, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

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

WojtekBoman Oct 18, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WojtekBoman commented Oct 16, 2023 •

edited

Loading

WojtekBoman Oct 18, 2023 •

edited

Loading