Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
1 of 6 tasks
m-natarajan opened this issue Sep 6, 2024 · 33 comments
Closed
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@m-natarajan
Copy link

m-natarajan commented Sep 6, 2024

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: 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

CleanShot.2024-09-06.at.18.35.54.mp4
Recording.520.mp4

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
  • Automatic offers:
    • ikevin127 | Reviewer | 103886812
Issue OwnerCurrent Issue Owner: @ikevin127
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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.

@alexpensify alexpensify added the External Added to denote the issue can be worked on by a contributor label Sep 6, 2024
@melvin-bot melvin-bot bot changed the title Top section of Settings doesn't remain fixed like we do on other pages [$250] Top section of Settings doesn't remain fixed like we do on other pages Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 6, 2024
Copy link

melvin-bot bot commented Sep 6, 2024

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

@gijoe0295
Copy link
Contributor

gijoe0295 commented Sep 6, 2024

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.

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

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.

contentContainerStyle={[styles.w100, styles.pt4]}

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

@gijoe0295
Copy link
Contributor

Update proposal

  • Add code permalinks

@Nodebrute
Copy link
Contributor

Nodebrute commented Sep 6, 2024

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

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

Move headerContent above scrollView


          {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
Screen.Recording.2024-09-07.at.12.09.28.AM.mov
Result with stickyindices
Screen.Recording.2024-09-07.at.12.12.35.AM.mov

What alternative solutions did you explore? (Optional)

@ikevin127
Copy link
Contributor

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

@ChavdaSachin
Copy link
Contributor

ChavdaSachin commented Sep 7, 2024

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]}

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

And

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

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

  • 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

Screenshot 2024-09-07 at 4 59 37 PM
<View style={[styles.ph5, styles.pb3 , styles.borderBottom]}>

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

Optional change

  • Scrollbar could be hidden, by passing additional ScrollView parameter value.
showsVerticalScrollIndicator = {false}

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

  • 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

Screenshot 2024-09-07 at 5 33 58 PM

Result With Bottom Border and Hidden Scrollbar

Screenshot 2024-09-07 at 5 28 04 PM

Result With only Hidden Scrollbar

Screenshot 2024-09-07 at 5 36 16 PM

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
Copy link
Contributor

Proposal

Updated Proposal
Added Postscript.

@mhynson
Copy link

mhynson commented Sep 7, 2024

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

expensify--issue-48736.mp4

Contributor details
Your Expensify account email: mike@mhdigitalsolutions.com
Upwork Profile Link: https://www.upwork.com/freelancers/~0178088c9fc5f2e566

Copy link

melvin-bot bot commented Sep 7, 2024

📣 @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.
    Screen Shot 2022-11-16 at 4 42 54 PM
    Format:
Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

@ikevin127
Copy link
Contributor

@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
Copy link
Contributor

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)
home p1 p2 p3

@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
Copy link
Contributor

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
Copy link
Contributor

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

@ikevin127
Copy link
Contributor

@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
Copy link
Contributor

Sounds like a plan! Thanks for the update.

@ikevin127
Copy link
Contributor

🎉 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
Screen.Recording.2024-09-09.at.11.39.37.mov

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

Copy link

melvin-bot bot commented Sep 9, 2024

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

@marcochavezf
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 9, 2024
Copy link

melvin-bot bot commented Sep 9, 2024

📣 @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

Copy link

melvin-bot bot commented Sep 9, 2024

📣 @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
Copy link
Contributor

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

@ChavdaSachin
Copy link
Contributor

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.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 9, 2024
@ikevin127
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

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

@ikevin127
Copy link
Contributor

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-19] according to today's production deploy from #48829 (comment).

cc @alexpensify @marcochavezf

@alexpensify
Copy link
Contributor

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

@alexpensify
Copy link
Contributor

I checked Upwork to confirm that we are ready for tomorrow. @ikevin127, you are set up, and I can complete the process tomorrow.

@ChavdaSachin—I tried searching your Upwork profile via the link in your GH bio, but the link is not active. I see you mentioned you input a proposal, but can you do so again here: https://www.upwork.com/jobs/~021832123307878163515

Thanks!

@ChavdaSachin
Copy link
Contributor

@alexpensify I have sent proposal please check.

@alexpensify
Copy link
Contributor

alexpensify commented Sep 19, 2024

Payouts due: 2024-09-19

Upwork job is here. All set; everyone has been paid via Upwork!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants