[HOLD for payment 2022-11-18] [$250] Workspace - No scroll bar to navigate a long list of members

[kavimuru]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Action Performed:

1. Open NewDot
2. Go to workspace > Manage members

Expected Result:

There should be a scroll bar to navigate to a member

Actual Result:

No scroll bar appears

Workaround:

unknown

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number: 1.2.22-1 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/199252923-a9d5dfd9-3bf4-4ee0-a4e4-15ad1fe8daf5.mp4

Expensify/Expensify Issue URL: Issue reported by: @marcaaron Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1667241349993479

View all open jobs on GitHub

[melvin-bot[bot]]

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

[syedsaroshfarrukhdot]

Proposal

If we need to add Vertical Scroll Bar In Manage Members We Need To Set showsVerticalScrollIndicator={true} in FlatList

https://github.com/Expensify/App/blob/151ec1ad3355cfa162ad490030f19a7134bbc076/src/pages/workspace/WorkspaceMembersPage.js#L315

After Fix :-

https://user-images.githubusercontent.com/81307212/199288734-f8d29bd5-821b-41b7-9004-35308171c7cd.mov

[melvin-bot[bot]]

Current assignee @tjferriss is eligible for the External assigner, not assigning anyone new.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[tjferriss]

Upworks job posting can be found here - https://www.upwork.com/jobs/~019e7d15837d51f5f4.

@rushatgabhane looks like we already have a proposal above. Can you review?

[0xmiroslav]

                            showsVerticalScrollIndicator={false}

Originally, this line was added by @puneetlath in this commit for https://github.com/Expensify/App/pull/5517 And I think it's better UI to hide vertical scroll bar on mobile app.

[devadarsann]

Like in the Android contacts applications, instead of scroll, show alphabets there and when the users scroll through the alphabets it will act like a scrollbar.

[ahmdshrif]

I think this scrollbar should be on the web only. and it's not a good thing to add in native. and in this case, we can make it like this. src/pages/workspace/WorkspaceMembersPage.js

 <FlatList
                              ...
      showsVerticalScrollIndicator={Os.platform == "Web"}
/>

or if we do not prefer to use Os.platfom

we can split the file into 2 files WorkspaceMembersPage.native.js and WorkspaceMembersPage.js

---

another way we can add scroll only if we do not have a touch screen.

so in this case . we can use it

import canUseTouchScreen from '../../libs/canUseTouchscreen';
...
   
   showsVerticalScrollIndicator={!canUseTouchScreen()}

[mountiny]

Asking in slack to clarify what is the desired outcome here https://expensify.slack.com/archives/C01GTK53T8Q/p1667481625045649?thread_ts=1667241349.993479&cid=C01GTK53T8Q

I think scrollbar should be there from accessibility perspective, but I agree this looks better without it.

[tjferriss]

Bumped the Slack thread to arrive at a conclusion.

[rushatgabhane]

Alright, the conclusion is to add a scrollbar.

But the scrollbar is overlapping the modal content. Is it possible to move it to the rightmost side of the modal?

[0xmiroslav]

Proposal

But the scrollbar is overlapping the modal content. Is it possible to move it to the rightmost side of the modal?

yes possible. This is because padding=20 applied to parent view. We can move this padding to scroll view (FlatList).

https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspaceMembersPage.js

-                   <View style={[styles.pageWrapper, styles.flex1]}>
+                   <View style={[styles.pageWrapper, styles.pb0]}>
                        <View style={[styles.w100, styles.flexRow]}>
                            <Button
                                medium
                                success
                                text={this.props.translate('common.invite')}
                                onPress={this.inviteUser}
                            />
                            <Button
                                medium
                                danger
                                style={[styles.ml2]}
                                isDisabled={this.state.selectedEmployees.length === 0}
                                text={this.props.translate('common.remove')}
                                onPress={this.askForConfirmationToRemove}
                            />
                        </View>
-                       <View style={[styles.w100, styles.mt4, styles.flex1]}>
+                       <View style={[styles.w100, styles.mt4]}>
                            <View style={[styles.peopleRow]}>
                                <View style={[styles.peopleRowCell]}>
                                    <Checkbox
                                        isChecked={this.state.selectedEmployees.length === removableMembers.length && removableMembers.length !== 0}
                                        onPress={() => this.toggleAllUsers()}
                                    />
                                </View>
                                <View style={[styles.peopleRowCell, styles.flex1]}>
                                    <Text style={[styles.textStrong, styles.ph5]}>
                                        {this.props.translate('workspace.people.selectAll')}
                                    </Text>
                                </View>
                            </View>
-                           <FlatList
-                               renderItem={this.renderItem}
-                               data={data}
-                               keyExtractor={item => item.login}
-                               showsVerticalScrollIndicator={false}
-                           />
                        </View>
                    </View>
+                   <FlatList
+                       renderItem={this.renderItem}
+                       data={data}
+                       keyExtractor={item => item.login}
+                       showsVerticalScrollIndicator
+                       style={[styles.ph5, styles.pb5]}
+                   />
                </FullPageNotFoundView>

https://github.com/Expensify/App/blob/main/src/styles/utilities/spacing.js

    pt5: {
        paddingTop: 20,
    },

+   pb0: {
+       paddingBottom: 0,
+   },
+
    pb1: {
        paddingBottom: 4,
    },

    pb2: {
        paddingBottom: 8,
    },

Demo:

[melvin-bot[bot]]

Auto-assign attempt failed, all eligible assignees are OOO.

[mountiny]

@shawnborton Will assign you later, but san you please confirm the looks of list with scrollbar? I am more worried about how this looks like on Windows, but I guess we dont have to worry about this now.

[syedsaroshfarrukhdot]

@rushatgabhane Yes, we can do that by refactoring styling on WorkspaceMembersPage.js as it is due to padding=20 applied to parent view.

https://github.com/Expensify/App/blob/main/src/pages/workspace/WorkspaceMembersPage.js

-                   <View style={[styles.pageWrapper, styles.flex1]}>
-                       <View style={[styles.w100, styles.flexRow]}>
+                   <View style={[styles.w100,styles.alignItemsCenter,styles.flex1]}>
+                      <View style={[styles.w100, styles.flexRow, styles.pt5, styles.ph5]}>
                            <Button
                                medium
                                success
                                text={this.props.translate('common.invite')}
                                onPress={this.inviteUser}
                            />
                            <Button
                                medium
                                danger
                                style={[styles.ml2]}
                                isDisabled={this.state.selectedEmployees.length === 0}
                                text={this.props.translate('common.remove')}
                                onPress={this.askForConfirmationToRemove}
                            />
                        </View>
                        <View style={[styles.w100, styles.mt4, styles.flex1]}>
-                            <View style={[styles.peopleRow]}>
+                            <View style={[styles.peopleRow, styles.ph5]}>
                                <View style={[styles.peopleRowCell]}>
                                    <Checkbox
                                        isChecked={this.state.selectedEmployees.length === removableMembers.length && removableMembers.length !== 0}
                                        onPress={() => this.toggleAllUsers()}
                                    />
                                </View>
                                <View style={[styles.peopleRowCell, styles.flex1]}>
                                    <Text style={[styles.textStrong, styles.ph5]}>
                                        {this.props.translate('workspace.people.selectAll')}
                                    </Text>
                                </View>
                            </View>
                            <FlatList
                                renderItem={this.renderItem}
                                data={data}
                                keyExtractor={item => item.login}
-                               showsVerticalScrollIndicator={false}
+                              showsVerticalScrollIndicator
+                              style={[styles.ph5,styles.pb5]}
                            />
                        </View>

After Fixing :-

https://user-images.githubusercontent.com/81307212/200559171-86c55e2d-0c29-45ed-997a-03ea6b9c8e84.mov

[melvin-bot[bot]]

Current assignee @shawnborton is eligible for the Design assigner, not assigning anyone new.

[mountiny]

@rushatgabhane @syedsaroshfarrukhdot Would these design changes influence the design on mobile devices? can you show screen of that as well so @shawnborton can confirm this looks fine? Thank you!

[syedsaroshfarrukhdot]

@mountiny No, they don't influence design on mobile devices. It look same across the board.

Desktop:-

https://user-images.githubusercontent.com/81307212/200606043-01dc185c-e251-4e1f-ba8f-185fd9b5a9be.mov

Web:-

https://user-images.githubusercontent.com/81307212/200606101-1aa4960c-35c4-4590-b539-afadc84349ad.mov

IOS:-

https://user-images.githubusercontent.com/81307212/200606156-3363de63-cf0c-47e9-baf3-daf2c4120a72.mov

mWEB Safari :-

https://user-images.githubusercontent.com/81307212/200606222-ec8aff92-38bf-478f-adea-89920096d2fa.mov

mWeb Chrome :-

https://user-images.githubusercontent.com/81307212/200606341-bc87af33-2577-4eb7-a0d7-b94d5ebeeaf5.mov

[melvin-bot[bot]]

📣 @syedsaroshfarrukhdot You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[mountiny]

I will go ahead and assign you @syedsaroshfarrukhdot. I think your solution does the job. Can you please raise the PR so we can test and see all how this change influences other designs.

[0xmiroslav]

@syedsaroshfarrukhdot 's proposal (apply padding:20 to each child) is just modified version of my proposal (move FlatList out of padding view)

padding:20 idea first came from me.

My proposal also fixes bottom view cut issue:

And I am the one who encouraged you to open discussion in slack to clarify what is the desired outcome.

cc: @mountiny

[syedsaroshfarrukhdot]

@mountiny Thanks for assigning. Applied on Upwork. Will be raising a PR in next hours.

[mountiny]

@syedsaroshfarrukhdot I am sorry but in this case I will have to accept I made a mistake and overlooked the similarities. Given @0xmiroslav has proposed solution which is very similar earlier, I think they should be assigned.

Apologies for the confusion, I have overlooked this.

@syedsaroshfarrukhdot I appreciate your help and sharing the videos and screenshots

[melvin-bot[bot]]

📣 @0xmiroslav You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[syedsaroshfarrukhdot]

@syedsaroshfarrukhdot I am sorry but in this case I will have to accept I made a mistake and overlooked the similarities. Given @0xmiroslav has proposed a solution which is very similar to earlier, I think they should be assigned.

Apologies for the confusion, I have overlooked this.

@syedsaroshfarrukhdot I appreciate your help and sharing the videos and screenshots

@mountiny Fundamentally both proposals are quite different from eachother @0xmiroslav is moving FlatList out of parent view whereas my proposal keeps the FlatList in the View and it fixes the issue without need of taking it out of the parent view. The difference b/w my proposal and @0xmirosla is quite significant.

Before:-

After My Fix :-

After @0xmiroslav Fix:-

The Last Option border is also cut in his proposal. So my proposal gets the exact view as it was before the fix with Scroll Bar.

@mountiny Please could you review both proposals and compare their differences and then assign the job to any of us ?

[mountiny]

@syedsaroshfarrukhdot Sorry but the proposals are very similar.

I have compared them even before and the only visual difference comes down to the padding at the bottom. The border line will most likely get cut off in you proposal to it just depends on how the row is aligned.

Hence that is the only design difference, I will defer to @shawnborton decision about the padding. It is not specified in the issue body, I dont mind it being removed.

@0xmiroslav Can you please wait a bit before we decide this so this does not feel fair to anyone, thanks.

[shawnborton]

I think I would just add a standard 20px padding to the very bottom of the list view so that the very last item always comes up fully into view.

[shawnborton]

So basically, this one looks correct to me:

Though we probably could slightly reduce the padding under the fixed Select All row.

[melvin-bot[bot]]

📣 @syedsaroshfarrukhdot You have been assigned to this job by @mountiny! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖