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 #6738

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

azimgd
Copy link
Contributor

@azimgd azimgd commented Dec 13, 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#3

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

…ng forked react-native-web version from expensify;
@azimgd azimgd requested a review from a team as a code owner December 13, 2021 20:06
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team December 13, 2021 20:06
@azimgd azimgd changed the title fix reverted list copy-paste results in wrong message order by applyi… Fix inverted list by applying forked version of react-native-web Dec 13, 2021
@azimgd
Copy link
Contributor Author

azimgd commented Dec 13, 2021

@thienlnam ready for review. First Expensify/react-native-web#3 needs to be merged.

@azimgd
Copy link
Contributor Author

azimgd commented Dec 13, 2021

@parasharrajat @PrashantMangukiya @kidroca

Based on your previous reports I would appreciate if you could take a look at this PR.

@azimgd
Copy link
Contributor Author

azimgd commented Dec 13, 2021

Fixes #6436 #6425

Copy link
Contributor

@kidroca kidroca left a comment

Choose a reason for hiding this comment

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

We already decided against having to build this in the postinstall hook
It would be much better to use a prebuilt hash from our fork

Build once use the same bundle everywhere vs making everyone build it themselves

cc @thienlnam

@azimgd
Copy link
Contributor Author

azimgd commented Dec 14, 2021

@kidroca could you link the thread with discussion here

@thienlnam
Copy link
Contributor

@azimgd
Copy link
Contributor Author

azimgd commented Dec 14, 2021

@thienlnam Simply changing main and dist won't work as the inner package reference becomes obsolete. I will and move rn-web into the root folder.

@azimgd
Copy link
Contributor Author

azimgd commented Dec 14, 2021

This still looks like a hack and a workaround while we could have implemented github's package service as a better approach (Proposition #1 from the thread).

@thienlnam
Copy link
Contributor

As discussed in the thread, going to allow this for now so we can have QA test these changes and create another issue to have it fixed.

Going to test this locally before approving / merging

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.

Tests well for me locally

@thienlnam thienlnam merged commit 1c80150 into Expensify:main Dec 14, 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.

@thienlnam
Copy link
Contributor

Linked the issue to update how we apply it #6763

For now, this can move along and we can have QA test the changes to the virtualized list

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @thienlnam in version: 1.1.20-3 🚀

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

@alex-mechler
Copy link
Contributor

Reverting to solve a deploy blocker #6792

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform result
🤖 android 🤖 failure ❌
🖥 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.

5 participants