[HOLD for payment 2024-02-07] [$500] Category & Tag - No scroll bar in Category and Tag list

lanitochka17 commented 8 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: 1.4.27-1 Reproducible in staging?: Y
Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: Applause - Internal Team Slack conversation:

Action Performed:

Precondition: User is an employee of Collect workspace that has categories and tags

Go to workspace chat
Open request money page
Enter amount > Next > Show more
Click Category
Scroll through the list
Click Tag
Scroll through the list

Expected Result:

There will be scroll bar when the list is long

Actual Result:

There is no scroll bar when the list is long

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/7d713b74-7d8a-4972-a718-746977301530

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01085ff3847931e9ee
Upwork Job ID: 1748069254741348352
Last Price Increase: 2024-01-18
Automatic offers:

akinwale | Reviewer | 28117079
bernhardoj | Contributor | 28117080

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

Job added to Upwork: https://www.upwork.com/jobs/~01085ff3847931e9ee

melvin-bot[bot] commented 8 months ago

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

lanitochka17 commented 8 months ago

#wave6-collect-submitters

FitseTLT commented 8 months ago

Proposal

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

Category & Tag - No scroll bar in Category and Tag list

What is the root cause of that problem?

We are not passing shouldAllowScrollingChildren to OptionsSelector in tag picker and category picker

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

We should be passing shouldAllowScrollingChildren true to OptionsSelector in tag picker and category picker

But additionally we need to fix BaseOptionsSelector too b/c now it doesn't have a scroll support when shouldTextInputAppearBelowOptions is false https://github.com/Expensify/App/blob/599c005849cd065a53d35e015991b637c768f2da/src/components/OptionsSelector/BaseOptionsSelector.js#L646-L662 So we should add a scroll view when shouldAllowScrollingChildren is true when shouldTextInputAppearBelowOptions is false same as when shouldTextInputAppearBelowOptions is true as in here https://github.com/Expensify/App/blob/599c005849cd065a53d35e015991b637c768f2da/src/components/OptionsSelector/BaseOptionsSelector.js#L632-L642 We should change it to

{/*
                     * 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.shouldAllowScrollingChildren && (
                        <ScrollView contentContainerStyle={[this.props.themeStyles.flexGrow1]}>
                            <ScrollView
                                horizontal
                                bounces={false}
                                contentContainerStyle={[this.props.themeStyles.flex1, this.props.themeStyles.flexColumn]}
                            >
                                {this.props.shouldTextInputAppearBelowOptions ? (
                                    optionsAndInputsBelowThem
                                ) : (
                                    <>
                                        <View style={this.props.shouldUseStyleForChildren ? [this.props.themeStyles.ph5, this.props.themeStyles.pb3] : []}>
                                            {this.props.children}
                                            {this.props.shouldShowTextInput && textInput}
                                            {Boolean(this.props.textInputAlert) && (
                                                <FormHelpMessage
                                                    message={this.props.textInputAlert}
                                                    style={[this.props.themeStyles.mb3]}
                                                    isError={false}
                                                />
                                            )}
                                        </View>
                                        {optionsList}
                                    </>
                                )}
                            </ScrollView>
                        </ScrollView>
                    )}
                    {!this.props.shouldAllowScrollingChildren &&
                        (this.props.shouldTextInputAppearBelowOptions ? (
                            optionsAndInputsBelowThem
                        ) : (
                            <>
                                <View style={this.props.shouldUseStyleForChildren ? [this.props.themeStyles.ph5, this.props.themeStyles.pb3] : []}>
                                    {this.props.children}
                                    {this.props.shouldShowTextInput && textInput}
                                    {Boolean(this.props.textInputAlert) && (
                                        <FormHelpMessage
                                            message={this.props.textInputAlert}
                                            style={[this.props.themeStyles.mb3]}
                                            isError={false}
                                        />
                                    )}
                                </View>
                                {optionsList}
                            </>
                        ))}

What alternative solutions did you explore? (Optional)

bernhardoj commented 8 months ago

Proposal

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

No scrollbar for category and tag list.

What is the root cause of that problem?

The list simply uses BaseOoptionsList that has a default value of false for showsVerticalScrollIndicator and we didn't pass it as true for both tag and category list, so the scroll is not visible. https://github.com/Expensify/App/blob/e6b07616db3a818574d60acf1d9ebaf1cb26bf02/src/components/OptionsList/BaseOptionsList.js#L279

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

I don't know what is the rule to decide which page should show a scrollbar or not because this happens on many pages, but the solution is just to pass showScrollIndicator to the list component (OptionsSelector), or make it default to true if we want to show the scroll on all pages.

aeioual commented 8 months ago

Proposal

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

Scroll bar is not visible for Category and Tag list

What is the root cause of that problem?

We do not pass the prop showScrollIndicator in BaseOptionsSelector component and hence we do not see the scrollbar by default in Category as well as Tag list (As well as all the places where OptionSelector is used)

https://github.com/Expensify/App/blob/f3111a472b72c475a89703fce4cdde4fdd75d42f/src/components/OptionsSelector/index.js#L5-L10

https://github.com/Expensify/App/blob/e6b07616db3a818574d60acf1d9ebaf1cb26bf02/src/components/TagPicker/index.js#L61

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

We just need to update the main BaseOptionsSelector component in OptionsSelector and pass showScrollIndicator prop (Default value of this prop is true and can be overwritten where ever requried).

<BaseOptionsSelector
        showScrollIndicator
       // eslint-disable-next-line react/jsx-props-no-spreading
        {...props}
        ref={ref}
        shouldTextInputInterceptSwipe
    />

Result

Screencast from 01-19-2024 11:39:17 AM.webm

What alternative solutions did you explore? (Optional)

N/A

akinwale commented 8 months ago

After reviewing all proposals, I recommend moving forward with @bernhardoj's proposal which contains a simple and straightforward solution.

@bernhardoj Let's try to identify all the list views in the Request money flow and apply the changes there.

@FitseTLT When making significant edits to your proposal, please follow the guidelines and create an updated post in the thread, irrespective of the fact that there is already another proposal posted or not. This makes things easier to keep track of. There is also a lengthy code diff without a clear explanation of what the change does.

🎀👀🎀 C+ reviewed.

melvin-bot[bot] commented 8 months ago

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

MariaHCD commented 8 months ago

The recommended proposal makes technical sense to me but I'd like an opinion from @Expensify/design regarding when the scroll bar should and shouldn't be displayed in the App. I'm not sure if it's intentional that the scroll bar isn't shown for these lists...

dannymcclain commented 8 months ago

@shawnborton do we ever purposely hide scrollbars in the app? I guess I had just assumed we display them whenever they're present.

(I'm always on Mac and iOS where scrollbars are always invisible until you're using them—so it's totally possible I've missed some scrollbar-related details.)

shawnborton commented 8 months ago

Hmm great question. I was under the same impression - we basically just follow platform conventions there, and indeed, on Mac I think they are basically hidden by default.

So I would say this isn't a bug and we can just close this one out.

akinwale commented 8 months ago

So I would say this isn't a bug and we can just close this one out.

I believe the bug here is that the scrollbars are not displayed at all even when scrolling.

shawnborton commented 8 months ago

Ah, got it, that makes sense. But that being said, whatever proposal we accept, we should make it so that the scroll behavior here matches the same scroll behavior elsewhere - we shouldn't just always show the scrollbar by default in this view, but rather, only when scrolling (if that's what the platform convention is of course).

aeioual commented 8 months ago

Considering the above conversation here is my updated proposal, : here

Ah, got it, that makes sense. But that being said, whatever proposal we accept, we should make it so that the scroll behavior here matches the same scroll behavior elsewhere - we shouldn't just always show the scrollbar by default in this view, but rather, only when scrolling (if that's what the platform convention is of course).

I have updated it to implement the scrollbar globally and not just for the mentioned view.

@akinwale, @MariaHCD

shawnborton commented 8 months ago

Can we confirm that proposal won't just force scrollbars to show on Mac even when scrolling is not happening?

aeioual commented 8 months ago

Yes, there are components like timezoneSelectPage which currently use the same prop and things work fine on Macbooks :)

neonbhai commented 8 months ago

Proposal

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

No scroll bar in Category and Tag list

What is the root cause of that problem?

We are actually missing a scroll indicator in many areas of the app:

Participants Selector

https://github.com/Expensify/App/assets/64629613/d975d33a-ab5e-428a-bc30-5a5c6907b0a3

Workspace Picker

https://github.com/Expensify/App/assets/64629613/d0ea8e8e-aa1b-45b7-abb0-63eb0b4df92f

Start Chat Page

https://github.com/Expensify/App/assets/64629613/773ec482-e697-44d3-8422-d2fd6d2661b1

Country List

https://github.com/Expensify/App/assets/64629613/c10e306e-a496-4665-96ee-e7ae6b57ecd3

State Picker

https://github.com/Expensify/App/assets/64629613/d790d119-b988-4bb6-8590-fa68f9fe8749

This is because, these lists in App are rendered by OptionsList and SelectionList. Both these lists have a prop showScrollIndicator that shows scroll bar (only when list is scrolling). But we turn these off by default: In BaseOptionsList: https://github.com/Expensify/App/blob/616d75f2e49be00f9bb0234cf2eb2cacf1522211/src/components/OptionsList/BaseOptionsList.tsx#L36

In BaseSelectionList: https://github.com/Expensify/App/blob/616d75f2e49be00f9bb0234cf2eb2cacf1522211/src/components/SelectionList/BaseSelectionList.js#L55

This then requires us to manually turn on the scroll bar (which hasn't been done for many lists). This is the root cause.

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

We should turn both these props here and here to true by default:

showScrollIndicator = true,

This will have the effect to show a scroll bar when a list is scrolling. Note that the scroll bar will not be visible when scroll is not happening.

This prop can be turned off for specific components if need be.

Since this prop is not turned on by default, we will remove it from being passed explicitly as passing for true would be unnecessary.

Result

https://github.com/Expensify/App/assets/64629613/0ba26f32-0f1b-4e1c-bb6a-b77200ec4780

akinwale commented 8 months ago

@neonbhai A proposal has already been selected.

@bernhardoj When testing, please take the comments in https://github.com/Expensify/App/issues/34762#issuecomment-1904827362 into consideration, especially regarding matching platform behaviour on macOS.

@MariaHCD I believe we can move forward here.

melvin-bot[bot] commented 8 months ago

📣 @akinwale 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 8 months ago

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer 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 📖

bernhardoj commented 8 months ago

PR is ready

cc: @akinwale

melvin-bot[bot] commented 8 months ago

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

melvin-bot[bot] commented 8 months ago

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

https://github.com/Expensify/App/pull/34953

If no regressions arise, payment will be issued on 2024-02-07. :confetti_ball:

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

@akinwale requires payment automatic offer (Reviewer)
@bernhardoj requires payment automatic offer (Contributor)

melvin-bot[bot] commented 8 months ago

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.
[ ] [@laurenreidexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

laurenreidexpensify commented 7 months ago

Payment Summary:

Contributor: $500 @bernhardoj paid in Upwork C+ review: $500 @akinwale paid in Upwork

laurenreidexpensify commented 7 months ago

remaining steps before closing: @akinwale to complete checklist and confirm re: regression testing

akinwale commented 7 months ago

[x] [@akinwale] The PR that introduced the bug has been identified. Link to the PR:

[x] [@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:

[x] [@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:

Not a regression. Scrollbars for the list items were not being displayed previously.

[x] [@akinwale] Determine if we should create a regression test for this bug.

[x] [@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

Launch Expensify in a browser
Click on the global create menu and select Request money
Select the Manual tab if it is not already active
Enter an amount and click Next
Click Category
Scroll through the list
Verify that the scrollbar (matching platform behaviour) is displayed (for lists which can scroll offscreen)
Select a Category or navigate back
Click Tag
Scroll through the list
Verify that the scrollbar (matching platform behaviour) is displayed (for lists which can scroll offscreen)

Do we agree 👍 or 👎?

akinwale commented 7 months ago

@laurenreidexpensify Payment received. Thanks.

I have completed the checklist.

Expensify / App