[HOLD for payment 2023-10-02] [$500] Implement new layout for initial settings page

[roryabraham]

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

---

Action Performed:

Open the settings page

Expected Result:

It should look like this:

Actual Result:

It looks like this:

Workaround:

n/a

Platforms:

all platforms

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~016d6da001b352374c
• Upwork Job ID: 1702172414692208640
• Last Price Increase: 2023-09-14
• Automatic offers:
• ishpaul777 | Contributor | 26670280

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @peterdbarkerUK (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

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

[melvin-bot]

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

[roryabraham]

Note that this is similar to the IllustratedHeaderPageLayout, but does not include an illustration. Perhaps we should create a simpler wrapper with just the layout, perhaps called VerticalSplitPageLayout, and then update IllustratedHeaderPageLayout to wrap that more generic component.

The top section of the initial settings screen should take up the same vertical height as the colored section for all other illustrated settings pages.

[ghost]

Can I work on it ?

[hungvu193]

Proposal

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

Implement new layout for initial settings page

What is the root cause of that problem?

We're using ScreenWrapper for IntinialSettingPage.

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

1. Add more props to IllustratedHeaderPageLayout call shouldUseIllustration (default to true) and CustomLayout.
2. Add a condition inside IllustratedHeaderPageLayoutto use CustomLayout incase shouldUseIllustration is false.
3. Create CustomLayout header to match the design of this issue.
4. Replace ScreenWraper of IntinialSettingPage and redudant component to use IllustratedHeaderPageLayout with the CustomLayout.

What alternative solutions did you explore? (Optional)

N/A

[DylanDylann]

Proposal

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

Implement new layout for initial settings page

What is the root cause of that problem?

This is a new feature

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

I suggest 2 solutions:
Solution 1:

• We still use IllustratedHeaderPageLayout and add more props called overlayContent (make sure that height of overlayContent is 240px to make sure the initial settings screen should take up the same vertical height as the colored section).
• In IllustratedHeaderPageLayout we check if overlayContent is not nil we add it instead of Lottie
• In InitialSettingsPage we pass overlayContent with related props

Solution 2:

• We will implement another component that is similar to IllustratedHeaderPageLayout but this component only accepts overlayContent from the parent component and doesn't have Lottie as mentioned here

What alternative solutions did you explore? (Optional)

[ishpaul777]

Proposal

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

Implement new layout for initial settings page

What is the root cause of that problem?

new feature, no root cause

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

We already have a component StaticHeaderPageLayout, We can modify StaticHeaderPageLayout to take a node as Header, and if header is in props it will use the header instead of the Image, below is rough implementation screenshot, ofc the styles need to be modified according to given specifications

What alternative solutions did you explore? (Optional)

[roryabraham]

My concern with @hungvu193's proposal is that is makes the IllustratedHeaderPageLayout more complex. I'd rather use composition than making the component more complex.

---

@DylanDylann's proposal #1

In IllustratedHeaderPageLayout we check if overlayContent is not nil we add it instead of Lottie

Some pages (such as the About page) need to show both the Lottie animation and the overlay.

---

@DylanDylann's proposal #2

We will implement another component that is similar to IllustratedHeaderPageLayout but this component only accepts overlayContent from the parent component and doesn't have Lottie as mentioned

Rather than just creating an entirely separate component, I'd rather we create a more generic layout component and then use it inside IllustratedHeaderPageLayout (i.e: component composition)

---

@ishpaul777's proposal

I think this makes sense given that we already have this component in place, but IMO we also need to DRY up the IllustratedHeaderPageLayout to use StaticHeaderPageLayout internally. @ishpaul777 If you agree and react 👍🏼 to this post and agree to make that change in your PR then I'll assign this to you.

[hungvu193]

I think this makes sense given that we already have this component in place, but IMO we also need to DRY up the IllustratedHeaderPageLayout to use StaticHeaderPageLayout internally. @ishpaul777 If you agree and react 👍🏼 to this post and agree to make that change in your PR then I'll assign this to you.

Agreed, I didn't know that we have StaticHeaderPageLayout, both StaticHeaderPageLayout and IllustratedHeaderPageLayout have almost the same structure and can be reuse.

[ishpaul777]

Agreed that IllustratedHeaderPageLayout need to be DRY up, IllustratedHeaderPageLayout has a similar structure and styling to StaticHeaderPageLayout we can use the StaticHeaderPageLayout in IllustratedHeaderPageLayout maybe rename StaticHeaderPageLayout to HeaderPageLayout. and pass illustration as a header node.

[ishpaul777]

@roryabraham you can checkout diffs - main...ishpaul777:App:feat/InitialSettingsPageLayout-change

I'll raise a PR on your 🟢.

Screen.Recording.2023-09-14.at.2.00.27.PM.mov

[ishpaul777]

@roryabraham bump on above ^

[ishpaul777]

@roryabraham friendly bump on clarifications on design specs

[roryabraham]

Hi @ishpaul777 sorry for the delayed response

What's the colour code of bg in the mockup

Please use sidebar theme color

Avatar Size looks larger in the mockup, currently we are using the variable Large for avatar looks like we need to make X-Large Size, what would be the height & width for that?

120x120

Mockup dont have a "<" back button and has "x" close is this intentional, we have "<" back button for other pages with similar layout

No, that's outdated. Please make sure we do not include the x close button.

[ishpaul777]

Thanks i'll make the the final changes by the end of today and mark PR open for review.

[melvin-bot]

Based on my calculations, the pull request did not get merged within 3 working days of assignment. Please, check out my computations here:

• when @ishpaul777 got assigned: 2023-09-14 09:17:10 Z
• when the PR got merged: 2023-09-21 00:25:55 UTC
• days elapsed: 4

On to the next one 🚀

[melvin-bot]

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

[melvin-bot]

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

• Feat/initial settings page layout change #27468

If no regressions arise, payment will be issued on 2023-10-02. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue — @ishpaul777 paid $750 (qualified for bonus)
• Contributor+ that helped on the issue and/or PR

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

• @ishpaul777 requires payment offer (Contributor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[CortneyOfstad]

@roryabraham are we good to close now that the contributor has been paid? TIA!

[ishpaul777]

Hii @roryabraham, curious if this should be eligible for bonus, as most part was done before the 3 business days only some final changes were awaited for your response.

[roryabraham]

Looks like your questions were answered 15 days ago, and the PR was merged 13 days ago.

So I suppose it makes sense that the bonus should apply. 👍

That said, there may have been more you could have done to get your questions answered faster, such as asking in the #expensify-open-source slack room. Maybe keep that in mind for next time in case you aren't getting fast responses in GitHub 🙂

[ishpaul777]

Thanks for you response, I asked question here https://expensify.slack.com/archives/C01GTK53T8Q/p1695055074085319

[CortneyOfstad]

Sounds good — @ishpaul777 I've re-hired you for the job in Upwork for the bonus amount of $250. Please let me know as soon as you accept so I can get that paid to you — thanks!

[ishpaul777]

Thank you✨, I have accepted the offer 😄

[CortneyOfstad]

Paid!