[$250] Top section of Settings doesn't remain fixed like we do on other pages

[m-natarajan]

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

---

Version Number: 9.0.30-11 Reproducible in staging?: Yes Reproducible in production?: Yes 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: @shawnborton Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1725640618731099

Action Performed:

1. Go to staging.new.expensify.com
2. Click Settings and scroll the LHN

Expected Result:

Top section of setting remain fixed (sticky) like other pages

Actual Result:

Top section of Settings is not sticky like we do on other pages

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

https://github.com/user-attachments/assets/e8d395c7-7647-497f-873f-ef0b7040570a

https://github.com/user-attachments/assets/98e7c4e8-5131-473f-82ec-6fe6be506485

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021832123307878163515
• Upwork Job ID: 1832123307878163515
• Last Price Increase: 2024-09-06

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot[bot]]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[gijoe0295]

Proposal

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

Top section of Settings is not sticky like we do on other pages

What is the root cause of that problem?

The account switcher is a normal component in the ScrollView.

https://github.com/Expensify/App/blob/67333b9a68f93f034c555e70920803253076400e/src/pages/settings/InitialSettingsPage.tsx#L436-L442

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

Add stickyHeaderIndices={[0]} to the ScrollView to make the first child sticky (i.e. the headerContent child).

Move styles.pt4 from ScrollView and add backgroundColor: theme.appBG to the headerContent wrapper style.

https://github.com/Expensify/App/blob/67333b9a68f93f034c555e70920803253076400e/src/pages/settings/InitialSettingsPage.tsx#L440

https://github.com/Expensify/App/blob/67333b9a68f93f034c555e70920803253076400e/src/pages/settings/InitialSettingsPage.tsx#L373

[gijoe0295]

Update proposal

• Add code permalinks

[Nodebrute]

Edited by proposal-police: This proposal was edited at 2024-09-06 19:09:22 UTC.

Proposal

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

Top section of Settings doesn't remain fixed like we do on other pages

What is the root cause of that problem?

We are adding headercontent in ScrollView https://github.com/Expensify/App/blob/67333b9a68f93f034c555e70920803253076400e/src/pages/settings/InitialSettingsPage.tsx#L442

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

Move headerContent above scrollView https://github.com/Expensify/App/blob/67333b9a68f93f034c555e70920803253076400e/src/pages/settings/InitialSettingsPage.tsx#L442

          {headerContent}
            <ScrollView
                ref={scrollViewRef}
                onScroll={onScroll}
                scrollEventThrottle={16}
                contentContainerStyle={[styles.w100]}
            >

We also need to move styles.pt4 from scrollView to headerContent styles.

[!NOTE]
Using stickyHeaderIndices={[0]} will cause the headerContent to be displayed with a scrollbar. You can see in attached video even in workspace list section the header is sticky but we don't show scrollbar.

Result with my solution

https://github.com/user-attachments/assets/1333654b-75c0-4493-8b8e-4706dd92873c

Result with stickyindices

https://github.com/user-attachments/assets/d408bb83-3baf-4dd2-b26d-1fe609c680c9

What alternative solutions did you explore? (Optional)

[ikevin127]

♻️ Will start reviewing the proposals in ~12h.

[ChavdaSachin]

Edited by proposal-police: This proposal was edited at 2024-09-07 12:55:13 UTC.

Proposal

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

• Settings page header is not fixed like on other pages.

What is the root cause of that problem?

• headerContent is included in the ScrollView here

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

• Move headerContent out of ScrollView and migrate top padding from from ScrollView to ScreenWrapper to retain top padding on same spot,
• So, change

style={[styles.w100, styles.pb0, styles.pt4]}

https://github.com/Expensify/App/blob/4af620b439636f714ff72a26320ed57f9d2faac3/src/pages/settings/InitialSettingsPage.tsx#L431 And

{headerContent}
<ScrollView
   ref={scrollViewRef}
   onScroll={onScroll}
   scrollEventThrottle={16}
   contentContainerStyle={[styles.w100]}
>
     

https://github.com/Expensify/App/blob/4af620b439636f714ff72a26320ed57f9d2faac3/src/pages/settings/InitialSettingsPage.tsx#L436-L442

• Additionally add bottom border to hearedContent to maintain consistent look on the page and to add an end point to the scrollbar.

Check this screenshot

<View style={[styles.ph5, styles.pb3 , styles.borderBottom]}>

https://github.com/Expensify/App/blob/4af620b439636f714ff72a26320ed57f9d2faac3/src/pages/settings/InitialSettingsPage.tsx#L373

Optional change

• Scrollbar could be hidden, by passing additional ScrollView parameter value.

showsVerticalScrollIndicator = {false}

https://github.com/Expensify/App/blob/4af620b439636f714ff72a26320ed57f9d2faac3/src/pages/settings/InitialSettingsPage.tsx#L436-L441

• Only hiding scrollbar without inserting bottom border to headerContent is also a considerable option (similar to Inbox LHN).

• Ps. Currently react-native hides scrollbar on ios and android so hiding scrollbar would additionally increase cross platform consistency. (In case scrollbar behaviour could be forced on ios and android if needed)

Result With Bottom Border

Result With Bottom Border and Hidden Scrollbar

Result With only Hidden Scrollbar

What alternative solutions did you explore? (Optional)

• stickyHeaderIndices={[0]} could be added to ScrollView along with showsVerticalScrollIndicator = {false} to hide scrollbar and add backgroundColor: theme.appBG to headerContent to avoid content overlapping. Optionally bottom border could be added to hederContent.

[ChavdaSachin]

Proposal

Updated Proposal Added Postscript.

[mhynson]

Edited by proposal-police: This proposal was edited at 2024-09-07 14:55:46 UTC.

Proposal

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

Top section of settings page does not remain fixed/sticky

What is the root cause of that problem?

{headerContent} is inside of the <ScrollView /> element in the InitialSettingsPage.tsx

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

• Move {headerContent} above the <ScrollView /> here
• remove the styles.pt4 from the <ScrollView/> here
• append styles.pt4 to the containing <View/> of headerContent here

Demo of Solution

https://github.com/user-attachments/assets/fc60a062-0e73-4fe2-8467-4b1ead9ef639

Contributor details Your Expensify account email: [email protected] Upwork Profile Link: https://www.upwork.com/freelancers/~0178088c9fc5f2e566

[melvin-bot[bot]]

📣 @mhynson! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[ikevin127]

@mhynson Thanks for your interest in contributing to the Expensify App. Please refer to the Contributing Guidelines before posting other proposals as you risk getting a warning if you don't follow to the guidelines. Specifically you are in violation of Propose a solution for the job rule number 6. (note) which states:

Note: Before submitting a proposal on an issue, be sure to read any other existing proposals. ALL NEW PROPOSALS MUST BE DIFFERENT FROM EXISTING PROPOSALS. The difference should be important, meaningful or considerable.

because your proposal is simply a copy of a previous existing proposal, without adding anything extra.

[!note] Repeated violations of the guidelines can lead to ⚠️ warnings, and if 3 warnings are received within a 1 year span leads to removal from the contributor program.

[ikevin127]

Based on the current proposals, 3 slightly different versions stand out:

Note: I'm comparing to the Home - Reports LHN since that was displayed as other pages in the issue OP videos.

Current Home - Reports LHN	Proposal 1 (full height LHN scrollbar)	Proposal 2 (scrollable only LHN scrollbar)	Proposal 3 (no scrollbar LHN)

@Expensify/design I need help deciding which version we want to go with from a design point of view. If we were to strictly go by the issues' OP and have this look like on the other pages then we would go for Proposal 3 (no scrollbar) but maybe we want something else so thought I should ask 😁

[!note]

@ Proposals authors

• to keep things fair, any edits of the existing proposals after this post won't change the final decision of the assignment based on design team's decision

[shawnborton]

Yeah, the third proposal makes sense to me assuming that we can still scroll that area, even without a scrollbar. Basically it should all work the same was as the Inbox page does with a fixed header and scrollable content area below it.

[alexpensify]

@ikevin127 - do you have enough feedback to carry on with the review? Thanks for checking!

[ikevin127]

@alexpensify Yes I do, but I want to check something regarding implementation before finalizing the review. It will be done in ~ 1 hour, as soon as I get to the office 👍

[alexpensify]

Sounds like a plan! Thanks for the update.

[ikevin127]

🎉 Thanks everybody for the proposals! Given that all proposals are similar in terms of RCA / solution, means that the final decision will be based on the smaller details.

@ChavdaSachin's proposal looks good to me because the RCA was identified and the solution seems the best choice in terms of design (see confirmation reference).

What I wanted to double-check was how the Home - Inbox LHN reports page looks if it were to have the scroll and it indeed looks like {headerContent} should be outside of the scroll list (see video below).

Video

https://github.com/user-attachments/assets/2cf329d0-7b74-42fc-9bcb-5007ce211239

When it comes to the PR, the only changes should be:

1. Moving {headerContent} outside of the ScrollView and ensuring the padding is correct (as seen before the change).
2. Hiding the scroll-bar.

🎀👀🎀 C+ reviewed

[melvin-bot[bot]]

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

[marcochavezf]

Sounds good, thanks for the review @ikevin127, assigning @ChavdaSachin 🚀

[melvin-bot[bot]]

📣 @ikevin127 🎉 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]]

📣 @ChavdaSachin You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[ChavdaSachin]

Thank you @ikevin127 and @marcochavezf for assigning me. Applied for the Upwork job and will raise PR within an hour or two.

[ChavdaSachin]

So I am making following changes,

1. migrating styles.pt4 from ScrollView to ScreenWrapper.
2. moving {headerContent} outside the ScrollView.
3. passing additional props showsVerticalScrollIndicator = {false} to ScrollView to hide scrollbar.

[ikevin127]

@ChavdaSachin Since this is your first job / contribution, here's some info on the process:

1. Contributor posts proposal.
2. Proposal is selected (your case here).
3. If contributor contributes for the first time, they have to apply to the Upwork job because they are not in the offer sending automation system yet (first job).
4. Contributor opens PR, pr is reviewed and if all good - merged.
5. PR will be deployed to staging and QA runs tests.
6. If no issues are reported, PR gets deployed to production ~1-2 days after staging deploy.
7. As soon as PR hits production, a 7-day regression period ensues and a HOLD for Payment {date} is added to the issue's title.
8. If no issues are reported at the end of the 7-day regression period, a BZ tram member will issue the payment for both Contributor and C+ | so you will get paid, no worries - it just has to follow this process.

[!note] In case at any stage of the deploy process, a regression is found coming from the deployed PR changes, the issue's bounty will be cut in half (-50%) for both controbutor and C+, and the regression period extends to when the follow-up fix PR is deployed to production.

When the payment is due date, the BZ team member which handles the payments will make sure you accept your offer and you get paid. Given the whole deploy and 7-days regression period process can take several days, the sooner the PR is opened, the better - but keep in mind that rushing to open the PR can lead to regressions if not tested thoroughly.

[ChavdaSachin]

@ikevin127 thanks a lot. I appreciate the way you operate.

In addition to above points do you have any note for me or did I miss anything ? Much appreciated.

[ikevin127]

@ChavdaSachin Sure, thanks! 🙏

You're all set, I am about to finish the review - the code in the PR looks and tests well. I will post my PR Reviewer Checklist soon and after that we're awaiting final review and merge from assigned Expensify engineer.

🎉 You did really good for your first PR in terms of the checklist, tests and code changes.

🗒️ The only thing that was missing (which is not a blocker) would be to add something in the Details section of your PRs, to give you some examples of some of my past PRs (ref1, ref2, ref3) - this helps the reviewers and engineers understand what the PR does and can help especially in cases where regressions are caused by a PR 👍

[ChavdaSachin]

Thanks @ikevin127, I will keep your advice on my mind.

[ikevin127]

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-19] according to today's production deploy from https://github.com/Expensify/App/pull/48829#issuecomment-2347263339.

cc @alexpensify @marcochavezf

[alexpensify]

Thanks, I've noted this date for the payment party.