[HOLD for payment 2024-06-05] [$500] Fix scrollable elements in Policy pages

[luacmartins]

Coming from this comment, all elements but the page header should scroll on policy pages, e.g. members, categories, tags. etc

chrome.mov

cc @Expensify/design

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010982d541dff55b4c
• Upwork Job ID: 1765759901307744256
• Last Price Increase: 2024-03-07
• Automatic offers:
• akinwale | Contributor | 0

Issue Owner

Current Issue Owner: @MitchExpensify

[melvin-bot]

Triggered auto assignment to @MitchExpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MitchExpensify]

So the whole page should scroll just like it does everywhere else, got it. I'll make this external tomorrow

[dukenv0307]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Only the list element can be scrolled on policy page

What is the root cause of that problem?

The SectionList is rendered as FlatList so it can be scrolled but we don't wrap WorkspaceMembersPage into a ScrollView and then it cannot be scrolled

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 520 in ffa731a

<View style={[styles.w100, styles.flex1]}>

What changes do you think we should make in order to solve the problem?

We can wrap all elements after the header like subtitle, ... or only SelectionList into a scroll view instead of a view and do the same way for other pages like category, tags, .... on policy pages

<ScrollView>
....
</ScrollView>

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 520 in ffa731a

<View style={[styles.w100, styles.flex1]}>

What alternative solutions did you explore? (Optional)

NA

[melvin-bot]

Current assignee @MitchExpensify is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010982d541dff55b4c

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[Krishna2323]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Fix scrollable elements in Policy pages

What is the root cause of that problem?

Feature request.

What changes do you think we should make in order to solve the problem?

We can't use ScrollView to wrap components content and because it also includes SelectionList and using SelectionList inside ScrollView will result in error (VirtualizedLists should never be nested inside plain ScrollViews). I think we should use ListHeaderComponent prop to pass the components in the header section of the page to SelectionList. We need to introduce a new prop ListHeaderComponent inside BaseSelectionList and then simply pass it to SectionList. We also need to make few minor changes, like we need to add the select all header after the ListHeaderComponent. We might only need this behaviour in small screen width devices so will implement it accordingly.

ListHeaderComponent={
    <>
        {ListHeaderComponent}
        {ListHeaderComponent && (
            <View style={[styles.peopleRow, styles.userSelectNone, styles.ph4, styles.pb3, listHeaderWrapperStyle]}>
                <Checkbox
                    isChecked={flattenedSections.allSelected}
                    onPress={selectAllRow}
                    disabled={flattenedSections.allOptions.length === flattenedSections.disabledOptionsIndexes.length}
                    accessibilityLabel={translate('workspace.people.selectAll')}
                />
                {customListHeader ?? (
                    <View style={[styles.flex1]}>
                        <Text style={[styles.textStrong, styles.ph3]}>{translate('workspace.people.selectAll')}</Text>
                    </View>
                )}
            </View>
        )}
    </>
}

Result

fix_scroll_view_workspace.mp4

Alternatively

A workaround to avoid native console error would be to use 2 scrollviews (horizontal ScrollView + FlatList) like in the code below

App/src/components/OptionsSelector/BaseOptionsSelector.js

Lines 620 to 628 in be716dd

	{/*
	* The OptionsList component uses a SectionList which uses a VirtualizedList internally.
	* VirtualizedList cannot be directly nested within ScrollViews of the same orientation.
	* To work around this, we wrap the OptionsList component with a horizontal ScrollView.
	*/}
	{this.props.shouldTextInputAppearBelowOptions && this.props.shouldAllowScrollingChildren && (
	<ScrollView contentContainerStyle={[this.props.themeStyles.flexGrow1]}>
	<ScrollView
	horizontal

[dragnoir]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Content on the header does not scroll with the list.

What is the root cause of that problem?

The issue is that only the Categories lis is nested into a scrollable component <SectionList>.

We can't just wrap the whole content including the <SectionList> into a ScrollView. This will lead to many performance issue and bacause:

The are several reasons why nested VirtualizedList is an Anti-pattern. Such as:

• The VirtualizedList cannot calculate the correct window size, because the ScrollView takes up the entire screen and does not constrain its content. Therefore, the VirtualizedList will try to render all the items at once, defeating the purpose of virtualization and potentially causing performance issues or crashes.
• The ScrollView will intercept all the touch events and prevent the VirtualizedList from handling them properly. This can affect features like pull-to-refresh, infinite scrolling, or swipe actions.
• The ScrollView will also interfere with the scroll position and momentum of the VirtualizedList, causing a janky and inconsistent user experience.

Also we can't just add a new new prop ListHeaderComponent inside BaseSelectionList and then simply pass it to SectionList. The SectionList is used by lots of other componets where we have headerMessage, search... if we pass the sticky part to ListHeaderComponent and in the future we need to enable the search input, the UI will get messy.

What changes do you think we should make in order to solve the problem?

1- On WorkspaceCategoriesPage create a new prop headerScrollable (true/false).

2- if true, then we render what we need from the header inside the [renderItem](https://github.com/Expensify/App/blob/537b7c081392c3721293989a9b6517f698580607/src/components/SelectionList/BaseSelectionList.tsx#L277) function with item 0

+ if (index  ===  0) {
  return (
    <View>
+     <NewHeaderCompoenent />
    <ListItem
    item={item}
    ....
    />
    </View>
    );
}

The <NewHeaderCompoenent /> will have all the content that we need it to be scrollable and at the top (like the +Add category and Settings buttons)

3- Adjust BaseSelectionList to hide/show elements as required if headerScrollable is true.

[MitchExpensify]

Some proposals here for you @aimane-chnaif 👍

[melvin-bot]

@MitchExpensify, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

[MitchExpensify]

Bumped @aimane-chnaif 1:1

[aimane-chnaif]

updating shortly

[aimane-chnaif]

ListHeaderComponent prop was introduced in react native for the exact purpose of fixing this kind of issue easily.
@Krishna2323's main solution looks good to me.
@Krishna2323 please make sure to apply general solution to all policy pages without much refactoring code.
🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @flodnv, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

This issue has not been updated in over 15 days. @akinwale, @flodnv, @MitchExpensify, @Krishna2323 eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[MitchExpensify]

Waiting on @Krishna2323 to fix merge conflicts

[flodnv]

PR is in final review

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.76-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• fix: Fix scrollable elements in Policy pages. #38719

If no regressions arise, payment will be issued on 2024-06-05. 🎊

For reference, here are some details about the assignees on this issue:

• @akinwale requires payment automatic offer (Contributor)
• @Krishna2323 requires payment (Needs manual offer from BZ)

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
• [@akinwale] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@akinwale] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@akinwale] Determine if we should create a regression test for this bug.
• [@akinwale] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@MitchExpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[MitchExpensify]

Reminder set to pay, @akinwale thoughts on the BZ steps?

[akinwale]

• [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
• [@akinwale] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@akinwale] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

This is a new feature.

• [@akinwale] Determine if we should create a regression test for this bug.
• [@akinwale] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps
Precondition: Members, Distance rates, Categories, Tags and Taxes screens should have multiple records to the point where the records can be scrolled

1. Launch the Expensify app
2. Select a Collect or Control workspace from the workspaces page

Devices with width greater than 800px

• Open any one of the Members, Distance rates, Categories, Tags or Taxes screens
• Verify that the options are scrollable on each page, and the options scroll as expected

Devices with width less than 800px

• Open any one of the Members, Distance rates, Categories, Tags or Taxes screens
• Verify that the options list scrolls properly on each screen
• Verify that the page description at the top and the thead scrolls while the buttons are fixed.

Do we agree 👍 or 👎?

[akinwale]

Reminder set to pay, @akinwale thoughts on the BZ steps?

@MitchExpensify Checklist completed.

[akinwale]

@MitchExpensify bump for payment. Thanks.

[MitchExpensify]

Paid and contract ended, thanks @akinwale !

[Krishna2323]

@MitchExpensify, I haven't been paid yet, automatic offer wasn't sent to me at the time of assignment.

[MitchExpensify]

Oh ok, I'm actually unsure what to do in that case. Are you expecting to be paid via Upwork or NewDot just to be sure?

[Krishna2323]

@MitchExpensify, through Upwork :)

[MitchExpensify]

Offer sent here Fix scrollable elements in Policy pages #37782

[Krishna2323]

@MitchExpensify, accepted.

[MitchExpensify]

Paid and contract ended! Thanks for your patience @Krishna2323