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 Mobile Web Scrolling in OptionsList #2101

Merged
merged 4 commits into from
Mar 26, 2021
Merged

Conversation

jasperhuangg
Copy link
Contributor

@jasperhuangg jasperhuangg commented Mar 26, 2021

Details

For some reason, the outer View in the OptionsList grows to fill the entire length of it's children on mobile (and therefore overflows the page), and therefore prevents scrolling.

I added height: 0 to this View (it still grows to fill its container since it has flex: 1) so that it doesn't overflow the document body. Ideally we can find a better solution that isn't as confusing/janky, I'll dig deeper when I have more time.

I've been unable to test it on iOS/Android (my dev env is broken), but I can verify that this doesn't break anything on Web, Mobile-Web, and Desktop.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/158391
Fixes https://github.com/Expensify/Expensify/issues/158390

Tests

  1. Log into an account with a large amount of contacts (enough to cause overflow when selecting a new chat).
  2. Verify that you are able to scroll.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Mobile Web

mobile_web_demo.mp4

Web

web_demo.mp4

Desktop

desktop_demo.mp4

@jasperhuangg jasperhuangg requested a review from a team as a code owner March 26, 2021 07:44
@jasperhuangg jasperhuangg self-assigned this Mar 26, 2021
@botify botify requested review from sketchydroide and removed request for a team March 26, 2021 07:44
@jasperhuangg
Copy link
Contributor Author

jasperhuangg commented Mar 26, 2021

The commit with message "found better solution" doesn't actually work, that's why I reverted it.

@nickmurray47 if your iOS/Android dev env works do you mind testing it on those platforms and updating the OP?

Also, I know this solution isn't super ideal (it's pretty confusing/janky), so I'll dig deeper when I have more time next week if you're not satisfied with this! We could deploy this in the meantime since it is breaking some pretty core functionality, so let me know what your thoughts are on this.

Copy link
Contributor

@sketchydroide sketchydroide left a comment

Choose a reason for hiding this comment

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

Change LGTM

@nickmurray47 nickmurray47 merged commit 7c1bb9f into master Mar 26, 2021
@nickmurray47 nickmurray47 deleted the jasper-fixMobileScrolling branch March 26, 2021 17:28
@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.


// need to set a height (0 works in this case) so that the view will scroll on mobile
// NOTE: the view will still fill its container since it has flex: 1 on it
<View style={[styles.flex1, {height: 0}]}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Heads up, inline styles are not permitted this should probably be moved to sizing.js.

Why does this work exactly? 🤔

@marcaaron
Copy link
Contributor

For some reason, the outer View in the OptionsList grows to fill the entire length of it's children on mobile (and therefore overflows the page), and therefore prevents scrolling. I added height: 0 to this View (it still grows to fill its container since it has flex: 1) so that it doesn't overflow the document body.

I'm pretty confused about why height: 0 works here.

Ideally we can find a better solution that isn't as confusing/janky, I'll dig deeper when I have more time

It's nice that this doesn't break anything, but yes, it would be ideal to avoid solutions that don't have clear explanations. This one is pretty bad because it defies reason and doesn't make much sense. If the height is set to 0 shouldn't this element be... invisible?

An alternative here could be to use the device height available in withWindowDimensions to set the total height we want this element to be.

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