Fix scrolling problem for LHN #2406

barun1997 · 2021-04-15T11:06:11Z

Details

I originally added flexGrow0 to the OptionsList because the view for IOUConfirmationList wasn’t showing the components as per the design. But flexGrow0 caused a regression in LHN because it stopped it from growing beyond its fixed height and width, and therefore, affected scrolling.

This solution solves the issue by using flexGrow0 only when it is passed as a prop. So components that need flex1 as a style in OptionsList have no regression.

Fixed Issues

The PR fixes the issue mentioned by the following comment:

#2120 (comment)

Tests

Go to IOUSplit page and continue until you reach IOUConfirm
Verify if all the rows and comments are shown.
Go to LHN, and verify you can scroll down the list

QA Steps

Same as Tests.

Tested On

Web
Mobile Web
iOS
Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

barun1997 · 2021-04-15T11:23:26Z

@Julesssss @marcaaron I resolved the regression passing a prop to the OptionsList, but I'm ideally looking for a better solution here. The problem, I think, is that - when you give flex: 1 to the OptionsList, it tries to take up all the available space, and thus, other children can't take available space. I tried different configurations to no avail. Maybe, I'm just not doing right, and my Flex knowledge is lagging me behind here! I'm open to any suggestions :)

marcaaron · 2021-04-15T17:39:23Z

I don't think this solution is working? I pulled down this branch and all the lists are still stuck and unable to move.

marcaaron · 2021-04-15T17:44:02Z

src/components/OptionsList.js

@@ -77,6 +80,7 @@ const defaultProps = {
    optionBackgroundColor: undefined,
    optionHoveredStyle: undefined,
    contentContainerStyles: [],
+    listContainerStyles: [],


Oh I see the issue now, if you give this a default value of undefined the solution will work because below you are checking:

this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1]

and empty array will be truthy value

Ahh my bad 😅

marcaaron · 2021-04-15T17:44:46Z

I think this is a fine solution as long as it causes no regressions and solves the original issue 🤷

marcaaron · 2021-04-15T17:45:43Z

Can you please add a link to the comment where the regression was discovered in the PR description?

Change the default value for listContainerStyles

github-actions · 2021-04-15T18:16:00Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

barun1997 · 2021-04-15T18:16:55Z

I have read the CLA Document and I hereby sign the CLA

barun1997 · 2021-04-15T18:20:44Z

recheck

barun1997 · 2021-04-15T19:15:40Z

Hi, @marcaaron! I made the change and also added the Comment in the PR description. :)

TomatoToaster

Hey I had one change requested. Let me know what you think!

TomatoToaster · 2021-04-15T22:31:19Z

src/components/OptionsList.js

@@ -196,7 +200,7 @@ class OptionsList extends Component {

    render() {
        return (
-            <View style={[styles.flexGrow0]}>
+            <View style={[...(this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1])]}>


I think this can be rewritten as

Suggested change

<View style={[...(this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1])]}>

<View style={[...(this.props.listContainerStyles || [styles.flex1])]}>

Though I'm a little unsure about the use of spread syntax here. Wouldn't [...(x)] be equal to just x?

I think this can be further simplified to:

Suggested change

<View style={[...(this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1])]}>

<View style={this.props.listContainerStyles || [styles.flex1]}>

Is there a specific reason you'd want to have the syntax in my comment above instead?

yeah if we want to make this super clean we can just do this

const defaultProps = { listContainerStyles: [styles.flex1], }; ... <View style={this.props.listContainerStyles}

@marcaaron I love this clean solution! Will implement it and push it. Thank you!

barun1997 · 2021-04-16T11:59:53Z

@Julesssss Made a slight change based on Marc's suggestion. Please feel free to review it any time! :)

Julesssss

LGTM

marcaaron

Looks like we are passing a 2D array now

TomatoToaster · 2021-04-16T15:48:26Z

src/components/OptionsList.js

@@ -196,7 +200,7 @@ class OptionsList extends Component {

    render() {
        return (
-            <View style={[styles.flexGrow0]}>
+            <View style={[this.props.listContainerStyles]}>


You're talking about this line right @marcaaron?

@TomatoToaster @marcaaron Damn, I am really sorry for the silly mistakes in this PR, guys. Looks like the festive season here has taken a toll on me! 😄

Thank you so much for your patience and the time to review this as well! :)

TomatoToaster

Thanks for fixing it promptly, LGTM.

isagoico · 2021-04-16T17:53:13Z

@marcaaron I see there are no QA steps. Do they need to be added or is this PR no qa?

Julesssss · 2021-04-19T08:48:02Z

The steps are currently testable internally (for simplicity). I'll run them now.

Julesssss · 2021-04-19T09:40:09Z

@barun1997 one thing that I've only just noticed is that you can't scroll the bill split contacts on the IOU Confirm page if they extend beyond the screen height. Was that always the case, or did that change with this PR? Thanks!

marcaaron · 2021-04-19T15:42:39Z

can't scroll the bill split contacts on the IOU Confirm page if they extend beyond the screen height

Sounds like what we were trying to fix with this PR 😅

OSBotify · 2021-04-19T22:41:56Z

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

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

roryabraham · 2021-04-23T01:15:13Z

@Julesssss Can we repeat QA for this tomorrow and get a more definitive answer? Please either:

Check it off on the deploy checklist, or
Label it as DeployBlockerCash and work towards a solution, or
Revert it and confirm that the QA issue is no longer present.

Thanks!

marcaaron · 2021-04-23T01:41:32Z

The LHN thing definitely was a blocker but that's resolved now so we can probably check this off.

Julesssss · 2021-04-23T09:53:40Z

Yeah, looks like someone has checked it off already. Thanks!

The remaining issue is in no way a blocker.

barun1997 · 2021-04-23T10:18:37Z

I will work on the non-scrolling issue over the weekend if it's okay? I have asked for help in the slack channel as well. In any case, will work extensively on it on Sunday!

barun1997 · 2021-04-24T03:00:53Z

@Jag96 I sent out an update here

Jag96 · 2021-04-26T20:16:20Z

Sounds good @barun1997, please let us know what progress you've made or what you're stuck on so we can help out. I suggest updating the slack thread with the latest progress and asking again in expensify-open-source for another look to get more eyes on it.

OSBotify · 2021-05-08T01:43:19Z

🚀 Deployed to production in version: 1.0.39-5🚀

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

Fix regression bug

70ce407

barun1997 force-pushed the fix-confirm-step-regression branch from 86f78e1 to 70ce407 Compare April 15, 2021 11:36

marcaaron reviewed Apr 15, 2021

View reviewed changes

marcaaron assigned barun1997 Apr 15, 2021

Update OptionsList.js

a566465

Change the default value for listContainerStyles

barun1997 marked this pull request as ready for review April 15, 2021 19:13

barun1997 requested a review from a team as a code owner April 15, 2021 19:13

MelvinBot requested review from TomatoToaster and removed request for a team April 15, 2021 19:13

TomatoToaster suggested changes Apr 15, 2021

View reviewed changes

Julesssss previously approved these changes Apr 16, 2021

View reviewed changes

Make code simpler

0cec32a

barun1997 dismissed Julesssss’s stale review via 0cec32a April 16, 2021 11:58

Julesssss previously approved these changes Apr 16, 2021

View reviewed changes

marcaaron requested changes Apr 16, 2021

View reviewed changes

TomatoToaster reviewed Apr 16, 2021

View reviewed changes

Fix 2D array

473e2ac

barun1997 dismissed Julesssss’s stale review via 473e2ac April 16, 2021 15:54

TomatoToaster approved these changes Apr 16, 2021

View reviewed changes

marcaaron approved these changes Apr 16, 2021

View reviewed changes

marcaaron merged commit 593b7d1 into Expensify:main Apr 16, 2021

OSBotify mentioned this pull request Apr 16, 2021

Deploy Checklist: Expensify.cash 2021-04-13 #2380

Closed

66 tasks

Jag96 mentioned this pull request Apr 19, 2021

[Chat] [IOU] Create IOUConfirm step #1988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix scrolling problem for LHN #2406

Fix scrolling problem for LHN #2406

barun1997 commented Apr 15, 2021 •

edited

Loading

barun1997 commented Apr 15, 2021

marcaaron commented Apr 15, 2021

marcaaron Apr 15, 2021

barun1997 Apr 15, 2021

marcaaron commented Apr 15, 2021

marcaaron commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 •

edited

Loading

barun1997 commented Apr 15, 2021

barun1997 commented Apr 15, 2021

barun1997 commented Apr 15, 2021

TomatoToaster left a comment

TomatoToaster Apr 15, 2021

TomatoToaster Apr 15, 2021 •

edited

Loading

marcaaron Apr 15, 2021

barun1997 Apr 16, 2021

barun1997 commented Apr 16, 2021

Julesssss left a comment

marcaaron left a comment

TomatoToaster Apr 16, 2021

barun1997 Apr 16, 2021

barun1997 Apr 16, 2021

TomatoToaster left a comment

isagoico commented Apr 16, 2021

Julesssss commented Apr 19, 2021

Julesssss commented Apr 19, 2021

marcaaron commented Apr 19, 2021

OSBotify commented Apr 19, 2021

roryabraham commented Apr 23, 2021

marcaaron commented Apr 23, 2021

Julesssss commented Apr 23, 2021 •

edited

Loading

barun1997 commented Apr 23, 2021

barun1997 commented Apr 24, 2021

Jag96 commented Apr 26, 2021

OSBotify commented May 8, 2021

	<View style={[...(this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1])]}>
	<View style={[...(this.props.listContainerStyles \|\| [styles.flex1])]}>

	<View style={[...(this.props.listContainerStyles ? this.props.listContainerStyles : [styles.flex1])]}>
	<View style={this.props.listContainerStyles \|\| [styles.flex1]}>

Fix scrolling problem for LHN #2406

Fix scrolling problem for LHN #2406

Conversation

barun1997 commented Apr 15, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

barun1997 commented Apr 15, 2021

marcaaron commented Apr 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcaaron commented Apr 15, 2021

marcaaron commented Apr 15, 2021

github-actions bot commented Apr 15, 2021 • edited Loading

barun1997 commented Apr 15, 2021

barun1997 commented Apr 15, 2021

barun1997 commented Apr 15, 2021

TomatoToaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomatoToaster Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barun1997 commented Apr 16, 2021

Julesssss left a comment

Choose a reason for hiding this comment

marcaaron left a 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

TomatoToaster left a comment

Choose a reason for hiding this comment

isagoico commented Apr 16, 2021

Julesssss commented Apr 19, 2021

Julesssss commented Apr 19, 2021

marcaaron commented Apr 19, 2021

OSBotify commented Apr 19, 2021

roryabraham commented Apr 23, 2021

marcaaron commented Apr 23, 2021

Julesssss commented Apr 23, 2021 • edited Loading

barun1997 commented Apr 23, 2021

barun1997 commented Apr 24, 2021

Jag96 commented Apr 26, 2021

OSBotify commented May 8, 2021

barun1997 commented Apr 15, 2021 •

edited

Loading

github-actions bot commented Apr 15, 2021 •

edited

Loading

TomatoToaster Apr 15, 2021 •

edited

Loading

Julesssss commented Apr 23, 2021 •

edited

Loading