Skip to content
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 inverted list by applying forked version of react-native-web #6268

Merged
merged 6 commits into from
Nov 16, 2021

Conversation

azimgd
Copy link
Contributor

@azimgd azimgd commented Nov 9, 2021

I used a patched version of react-native-web with fixes for copying text into the buffer in reversed order;

React native change: Expensify/react-native-web@bb711d2

Details

I have removed flatlist inversion (by inverted=true prop) which used [transform: translate] css hack and instead adding flatlist array items in reverse order [unshift instead of push]; It also reverses ScrollView event response received from browser [e.nativeEvent.configOffset *= -1];

Fixed Issues

#3381

Tests

  1. run npm install which will patch react-native-web package
  2. npm run web
  3. load a chat with 200 messages
  4. scrolling up and down (with debugger enabled) works fine
  5. copying text into buffer preserves order

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-08-09.at.01.16.16.mov

Mobile Web

Desktop

iOS

Android

@azimgd azimgd requested a review from a team as a code owner November 9, 2021 17:24
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team November 9, 2021 17:24
@azimgd
Copy link
Contributor Author

azimgd commented Nov 9, 2021

outdated PR: #5561

@thienlnam
Copy link
Contributor

thienlnam commented Nov 9, 2021

Testing but app is crashing when I scroll quickly
Screen Shot 2021-11-09 at 1 07 52 PM
Screen Shot 2021-11-09 at 1 08 02 PM

Edit: Noticed this happening on main as well - doesn't seem related

thienlnam
thienlnam previously approved these changes Nov 9, 2021
@thienlnam thienlnam changed the title Fix inverted list by applying forked version of react-native-web [Company Offsite Hold] Fix inverted list by applying forked version of react-native-web Nov 9, 2021
@azimgd
Copy link
Contributor Author

azimgd commented Nov 16, 2021

@thienlnam ready to be merged ?

@thienlnam
Copy link
Contributor

Could you address the comment here ? #3381 (comment)

@thienlnam
Copy link
Contributor

The change would be moving the commands you have currently in your postinstall into a seperate file called /build/react-native-web.sh, and then the postinstall would just be postinstall: './build/react-native-web.sh'

@azimgd
Copy link
Contributor Author

azimgd commented Nov 16, 2021

Done @thienlnam

@thienlnam
Copy link
Contributor

Screen Shot 2021-11-16 at 10 39 30 AM

I think you need to add execute permissions to the file https://stackoverflow.com/questions/42154912/permission-denied-for-build-sh-file

@azimgd
Copy link
Contributor Author

azimgd commented Nov 16, 2021

@thienlnam try now

@thienlnam thienlnam changed the title [Company Offsite Hold] Fix inverted list by applying forked version of react-native-web Fix inverted list by applying forked version of react-native-web Nov 16, 2021
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Awesome, tests well for me locally - shipping it and 🤞 we don't cause regressions

@thienlnam
Copy link
Contributor

Running tests

@thienlnam thienlnam merged commit 8d5a3ad into Expensify:main Nov 16, 2021
@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

🚀 Deployed to staging by @thienlnam in version: 1.1.15-18 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.16-10 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @thienlnam in version: 1.1.16-11 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Dec 6, 2021

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

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

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.

3 participants