-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Comments
Triggered auto assignment to @alexpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~021832123307878163515 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 ( |
ProposalPlease 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. App/src/pages/settings/InitialSettingsPage.tsx Lines 436 to 442 in 67333b9
What changes do you think we should make in order to solve the problem?Add Move
|
Update proposal
|
Edited by proposal-police: This proposal was edited at 2024-09-06 19:09:22 UTC. ProposalPlease 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
We also need to move 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 solutionScreen.Recording.2024-09-07.at.12.09.28.AM.movResult with stickyindicesScreen.Recording.2024-09-07.at.12.12.35.AM.movWhat alternative solutions did you explore? (Optional) |
♻️ Will start reviewing the proposals in ~12h. |
Edited by proposal-police: This proposal was edited at 2024-09-07 12:55:13 UTC. ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?
What changes do you think we should make in order to solve the problem?
style={[styles.w100, styles.pb0, styles.pt4]}
And {headerContent}
<ScrollView
ref={scrollViewRef}
onScroll={onScroll}
scrollEventThrottle={16}
contentContainerStyle={[styles.w100]}
>
App/src/pages/settings/InitialSettingsPage.tsx Lines 436 to 442 in 4af620b
<View style={[styles.ph5, styles.pb3 , styles.borderBottom]}>
showsVerticalScrollIndicator = {false} App/src/pages/settings/InitialSettingsPage.tsx Lines 436 to 441 in 4af620b
What alternative solutions did you explore? (Optional)
|
ProposalUpdated Proposal |
Edited by proposal-police: This proposal was edited at 2024-09-07 14:55:46 UTC. ProposalPlease 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?
What changes do you think we should make in order to solve the problem?
Demo of Solutionexpensify--issue-48736.mp4Contributor details |
📣 @mhynson! 📣
|
@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:
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 |
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. |
@ikevin127 - do you have enough feedback to carry on with the review? Thanks for checking! |
@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 👍 |
Sounds like a plan! Thanks for the update. |
🎉 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 VideoScreen.Recording.2024-09-09.at.11.39.37.movWhen it comes to the PR, the only changes should be:
🎀👀🎀 C+ reviewed |
Triggered auto assignment to @marcochavezf, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
Sounds good, thanks for the review @ikevin127, assigning @ChavdaSachin 🚀 |
📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @ChavdaSachin You have been assigned to this job! |
Thank you @ikevin127 and @marcochavezf for assigning me. |
So I am making following changes,
|
@ChavdaSachin Since this is your first job / contribution, here's some info on the 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. |
@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 ? |
@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 👍 |
Thanks @ikevin127, I will keep your advice on my mind. |
|
Thanks, I've noted this date for the payment party. |
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! |
@alexpensify I have sent proposal please check. |
Payouts due: 2024-09-19
Upwork job is here. All set; everyone has been paid via Upwork! |
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:
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?
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
Issue Owner
Current Issue Owner: @ikevin127The text was updated successfully, but these errors were encountered: