Refactor: Remove Avatar platform specific code by kidroca · Pull Request #15663 · Expensify/App

kidroca · 2023-03-04T18:26:08Z

Details

The avatar component uses a workaround to prevent a flicker bug in react-native-web
We submitted a fix for react-native-web and the workaround is no longer needed

Why was the Image component updated

The code contributed to the flicker because the imageSource starts with undefined and is only updated after mount - this causes the first render pass to show no image (this happens very quickly, but sometimes a flicker can be seen)

Fixed Issues

$ #14278
PROPOSAL: #9183 (comment)
FORK PR: Expensify/react-native-web#15

Tests

There should be no flicker

Log into an account on New Expensify
Navigate to a chat where you were the last person to send a message
Send a message in that chat
Confirm that the avatar on the left hand side of the message that you just sent does not flickers.

Image `onLoad` was slightly changed, it should not cause a regression

Thumbnails should work as before / resize correctly after load
Attachment preview modals should work as before

Offline tests

Sending a message while offline should not case the avatar to flicker

QA Steps

Log into an account on New Expensify
Navigate to a chat where you were the last person to send a message
Send a message in that chat
Confirm that the avatar on the left hand side of the message that you just sent does not flickers.

Thumbnails should work as before / resize correctly after load
Attachment preview modals should work as before

PR Author Checklist

Screenshots/Videos

Web

Screen.Recording.2023-03-04.at.19.48.03.mov

Mobile Web - Chrome

Android.Emulator.-.Pixel_2_API_29_5554.2023-03-06.15-35-04.mp4

Mobile Web - Safari

Desktop

Screen.Recording.2023-03-04.at.20.40.49.mov

iOS

Simulator.Screen.Recording.-.iPhone.14.-.2023-03-04.at.20.47.43.mp4

Android

Android.Emulator.-.Pixel_2_API_29_5554.2023-03-06.15-30-22.mp4

kidroca

Added some notes to aid the reviewer

kidroca · 2023-03-04T19:10:00Z

package.json

This is the complied code containing the flicker fix
After it's merged the package should be updated to point to the new version

Fix Image Caching react-native-web#15

should we throw a hold on this PR until Expensify/react-native-web#15 is merged?

should we throw a hold on this PR until Expensify/react-native-web#15 is merged?

Yes, added HOLD to the title

kidroca · 2023-03-04T19:11:51Z

src/components/Avatar.js

The defaultSource workaround is no longer needed.
We just pass source

kidroca · 2023-03-04T19:15:51Z

src/components/Image/index.js

Having imageSource with undefined value initially would render no image for the first render

We don't have to use state for imageSource - it doesn't save us from re-renders, because most usages pass source like <Image soruce={{ uri: '...' }} /> (a new object created inline)

The debounce here was removed because it no longer has any effect (and was a workaround) as discussed here: #9279 (comment)

(Also it's inconsistent with the .native component where no debounce is used)

There are further changes that we can add to the Image component and I can open a follow up PR:

remove configureOnLoad - this was a workaround needed because at the time react-native-web did not provide dimension in the original onLoad callback

Turn Image into a functional component - no lifecycle and state are used.

I do both of this changes here:

Image Web/Desktop: Add support for http headers #13036

but the PR is on hold

MelvinBot · 2023-03-04T19:17:36Z

@mollfpr @bondydaa One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/components/Image/index.js

mollfpr · 2023-03-06T15:06:14Z

Reviewer Checklist

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- Android / native
- Android / Chrome
- iOS / native
- iOS / Safari
- MacOS / Chrome / Safari
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Web

15663.Web.mov

15663.Web.2.mp4

Mobile Web - Chrome

15663.mWeb-Chrome.mp4

Mobile Web - Safari

15663.mWeb.Safari.mp4

15663.mWeb-Safari.2.mp4

Desktop

15663.Desktop.mp4

iOS

15663.iOS.2.mp4

Android

15663.Android.mp4

15663.iOS.mp4

mollfpr

Great work on this @kidroca! Looks good to me and tests well 👍

kidroca · 2023-03-06T17:41:37Z

Thanks @mollfpr
I just want to remind that the current PR would have wait and be updated with the new @expensify/react-native-web version, once it is merged:

Fix Image Caching react-native-web#15

BTW no one is assigned to review it

Current PR servers to verify the fix is working in App, but it has to wait for the actual fix to be released in @expensify/react-native-web

bondydaa

👍 looks good, I'll approve once we've gotten the react-native-web PR merged and the package.json updated with the correct version number.

mountiny

I am going to try to release the new version of our fork, @kidroca can you resolve the merge conflicts in the meantime, thanks!

…t-avatar-icon-from-flicker" This reverts commit 92186e5, reversing changes made to 99def93.

kidroca · 2023-03-22T07:06:15Z

I am going to try to release the new version of our fork

I don't see the new @expensify/react-native-web version, latest is v0.18.12 - published 2 months ago: https://www.npmjs.com/package/@expensify/react-native-web

mountiny · 2023-03-23T15:37:59Z

@kidroca should be released in version 0.18.14

kidroca · 2023-03-23T18:51:23Z

Updated to latest @expensify/react-native-web package (including the fix)

@bondydaa you approved this PR before but there's a change that I had to make: https://github.com/Expensify/App/pull/15663/files#r1144494327

✅ Ready for Review

mollfpr · 2023-03-24T15:34:56Z

Screenshots/Videos

Web

15663.Web.mp4

Mobile Web - Chrome

15663.mWeb-Chrome.mp4

Mobile Web - Safari

15663.mWeb-Safari.mp4

Desktop

15663.Desktop.mp4

iOS

15663.iOS.mp4

Android

15663.Android.mp4

mollfpr

Test well 👍

mountiny

Thank you very much, going to go ahead and merge given Bondy has approved this before. Thank for quick work @kidroca and thanks for testing @mollfpr

OSBotify · 2023-03-24T16:15:09Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2023-03-24T19:05:35Z

🚀 Deployed to staging by https://github.com/mountiny in version: 1.2.89-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2023-03-27T10:32:17Z

🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.89-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

kidroca mentioned this pull request Mar 4, 2023

Fix Image Caching Expensify/react-native-web#15

Merged

kidroca force-pushed the kidroca/refactor/avatar-platform-specific-code-replacement branch from a267ce4 to ec58ab9 Compare March 4, 2023 19:10

kidroca commented Mar 4, 2023

View reviewed changes

kidroca marked this pull request as ready for review March 4, 2023 19:17

kidroca requested a review from a team as a code owner March 4, 2023 19:17

kidroca mentioned this pull request Mar 4, 2023

[HOLD for payment 2023-04-03] Clean-up react-native-web workaround and fix it upstream #14278

Closed

melvin-bot bot requested review from bondydaa and mollfpr and removed request for a team March 4, 2023 19:17

mollfpr reviewed Mar 5, 2023

View reviewed changes

src/components/Image/index.js Outdated Show resolved Hide resolved

mollfpr previously approved these changes Mar 6, 2023

View reviewed changes

bondydaa reviewed Mar 8, 2023

View reviewed changes

kidroca changed the title ~~Refactor: Remove Avatar platform specific code~~ [HOLD: https://github.com/Expensify/react-native-web/pull/15] Refactor: Remove Avatar platform specific code Mar 9, 2023

This was referenced Mar 17, 2023

adding carousel to attachment modal #9279

Merged

[HOLD for payment 2023-04-27] [$1000] The attachment viewer loads every image again when switching with arrows #15922

Closed

mountiny assigned kidroca Mar 21, 2023

mountiny self-requested a review March 21, 2023 12:44

mountiny reviewed Mar 21, 2023

View reviewed changes

kidroca added 2 commits March 22, 2023 08:58

Revert "Merge pull request Expensify#9995 from b1tjoy/fix/9183-preven…

1519e28

…t-avatar-icon-from-flicker" This reverts commit 92186e5, reversing changes made to 99def93.

Update Image to not render briefly with undefined source

4c53413

kidroca dismissed mollfpr’s stale review via 33560af March 22, 2023 09:34

kidroca force-pushed the kidroca/refactor/avatar-platform-specific-code-replacement branch from e335e60 to 33560af Compare March 22, 2023 09:34

mountiny mentioned this pull request Mar 23, 2023

[$1000] Scrolling speed is 2 to 3 times faster compared to other apps #15759

Closed

6 tasks

Image - omit props that shouldn't be forwarded

9bb4ac7

kidroca force-pushed the kidroca/refactor/avatar-platform-specific-code-replacement branch from 33560af to d3a0794 Compare March 23, 2023 18:14

Use @expensify/react-native-web v0.18.15

c4abb46

kidroca force-pushed the kidroca/refactor/avatar-platform-specific-code-replacement branch from d3a0794 to c4abb46 Compare March 23, 2023 18:46

mollfpr approved these changes Mar 24, 2023

View reviewed changes

mountiny changed the title ~~[HOLD: https://github.com/Expensify/react-native-web/pull/15] Refactor: Remove Avatar platform specific code~~ Refactor: Remove Avatar platform specific code Mar 24, 2023

mountiny approved these changes Mar 24, 2023

View reviewed changes

mountiny merged commit 07a3796 into Expensify:main Mar 24, 2023

OSBotify mentioned this pull request Mar 24, 2023

Deploy Checklist: New Expensify 2023-03-24 #16489

Closed

44 tasks

kidroca deleted the kidroca/refactor/avatar-platform-specific-code-replacement branch March 24, 2023 18:10

tienifr mentioned this pull request Mar 28, 2023

[$2000] Previously opened chat profile pic appears for second when switch between chats #15962

Closed

6 tasks

aimane-chnaif mentioned this pull request Mar 31, 2023

[$1000] Cursor appears zoom cursor for a moment and changes to normal cursor when opening profile pic from details page #16100

Closed

6 tasks

Conversation

kidroca commented Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Why was the Image component updated

Fixed Issues

Tests

There should be no flicker

Image onLoad was slightly changed, it should not cause a regression

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

kidroca Mar 4, 2023

Choose a reason for hiding this comment

Uh oh!

bondydaa Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

kidroca Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

kidroca Mar 4, 2023

Choose a reason for hiding this comment

Uh oh!

kidroca Mar 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

MelvinBot commented Mar 4, 2023

Uh oh!

Uh oh!

mollfpr commented Mar 6, 2023

Reviewer Checklist

Screenshots/Videos

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

kidroca commented Mar 6, 2023

Uh oh!

bondydaa left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

kidroca commented Mar 22, 2023

Uh oh!

mountiny commented Mar 23, 2023

Uh oh!

kidroca commented Mar 23, 2023

Uh oh!

mollfpr commented Mar 24, 2023

Screenshots/Videos

Uh oh!

mollfpr left a comment

Choose a reason for hiding this comment

Uh oh!

mountiny left a comment

Choose a reason for hiding this comment

Uh oh!

OSBotify commented Mar 24, 2023

Uh oh!

OSBotify commented Mar 24, 2023

Uh oh!

OSBotify commented Mar 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kidroca commented Mar 4, 2023 •

edited

Loading

Image `onLoad` was slightly changed, it should not cause a regression

kidroca Mar 4, 2023 •

edited

Loading