[$500] Fix style regressions caused by new Global Nav component

[kbecciv]

This issue handles all regressions stemmed from the addition if the new Global Nav component in this PR. Any proposals should handle all these regressions. Please refer to the screenshots attached for the new design requirements to solve these issues.

cc: @allroundexperts

• Sign in button on Global Nav is cropped (the original issue)

  

• Avatar on Global Nav is cropped

  

• Avatar on Global Nav is very close to the native icons on desktop

• Settings Background for profile details Header is changed unintentionally while refactoring LHN

• Sign in banner for anonymous user don't have a highlighted green background color

[namhihi237]

Proposal

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

Sign in button and avatar with status should fit into the Sub LHN

What is the root cause of that problem?

Currently, global navigation has width 72px https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/styles/variables.ts#L83-L84

But the button sign-in has width 80px. https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/styles/styles.js#L2296-L2298

When using set status we have sidebarStatusAvatarContainer width 84px so it will crop the avatar

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

we can increase the width of global navigation to 86px or more, I think 92 is better, and we should center the float button. The better we should wait for the design for that.

What alternative solutions did you explore? (Optional)

N/A

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[ishpaul777]

Proposal

Problem

Sign in button for anonymous user on New LHN Nav is not fitting properly.

Problem

Sign in Button has a width of 82 px while the globalNavigation container has width of 70 px so the sign in button does not fit in container

Changes

We can increase the size for the globalNavigation container to fit the sign in button but that would decrease the size for the container for chats, especially for small screen devices.

Also the decreasing the size for sign in button won't look appealing.

IMO, we should not the change the position for the sign in button and keep it next to the Search Icon. We can remove the avatar when user is not signed in or have the avatar but the onPress action should redirect sign in button we can use Session.checkIfActionIsAllowed for this.

edit - Just noticed that the same issue is reproduced if signed in user sets a status the staus and avatar also not fit in the container. I think we should let the Design team decide the expected behaviour here

[ZhenjaHorbach]

Proposal

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

The button is too big for sidebar

What is the root cause of that problem?

Sidebar width(72) is smaller than the button(80)

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

First we need to look at the sidebar styles

https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/pages/home/sidebar/GlobalNavigation/index.js#L32

The most interesting thing for us is styles.ph5 and styles.globalNavigation

When these styles were created, perhaps the idea was that the width of the sidebar would be 72(styles.globalNavigation) + 20/20(styles.ph5)

But since in react native box-sizing is equal to border-box We get the width which equals only 72

And if my theory is correct Then in this case we need to increase the width to 112 (but 92 is better ) In which 72 is content and 20+20(10+10) is the border

What alternative solutions did you explore? (Optional)

The best way to fix this is to first contact the designers ))))

[ashishBajracharyaFrostidi]

the sidebar has a fixed width which is causing this issue, giving a variable width to the sidebar relative to the button width will solve this issue, even in the future if the button text changes or the font size increases, it won't cause a problem. #simpleCSSHacks

[melvin-bot[bot]]

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

[ashishBajracharyaFrostidi]

Contributor details Your Expensify account email: ashish.bajracharya.frostdigi.ai@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/~0135bbe272a370da40

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[maxconnectAbhi]

Proposal

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

Login - Sign in button for anonymous user on New LHN Nav is not fitting

What is the root cause of that problem?

Sign in Button has a width of 82 px while the globalNavigation container has width of 70 px so the sign in button does not fit in container

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

We should make the width for sign in button 95% of the container here width: '95%' https://github.com/Expensify/App/blob/389d7b0c4b96c6f2a2295055c685791d35b65252/src/styles/styles.js#L2296-L2298

What alternative solutions did you explore? (Optional)

NA

[hayata-suenaga]

🚨 Can you combine this issue with this issue?

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[hayata-suenaga]

melvin, please remove the overdue label.

[hayata-suenaga]

assigning myself as this is a regression from a wave 8 issue

[melvin-bot[bot]]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[hayata-suenaga]

OP was changed. Please update your proposals to reflect the new requirements 🙇

[b4s36t4]

Proposal

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

Fix style regressions caused by new Global Nav component

What is the root cause of that problem?

1. Sign in button on Global Nav is cropped

Previously we have the Signin button at the top, now we need to replace it with the expensify logo. https://github.com/Expensify/App/blob/0cbee2247e48c17eec428b989afead3dc6f196d4/src/pages/home/sidebar/SignInOrAvatarWithOptionalStatus.js#L40 in this file we need to make changes and remove Signin Button also remove the component as at this only file.

2. Avatar on Global Nav is cropped

Avatar on glbal nav is only cropped because of the width issue which only happens if we have custom status.

3. Avatar on Global Nav is very close to the native icons on desktop

Previously we used to have same bg color on the header so it won't be visible. After added the new bg over the global we have different bg colors.

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

1. Update SignInButton https://github.com/Expensify/App/blob/0cbee2247e48c17eec428b989afead3dc6f196d4/src/pages/home/sidebar/SignInOrAvatarWithOptionalStatus.js in this file to use Expensify logo.

To support the new design where we show SignIn button at the bottom on LHN we can existing component AnonymousReportFooter which might need some tweaks as that component is expecting report as prop.

We don't need to worry about the multiple items covering the LHN as a anonymous will be having only one chat at time.

2. Use position property to update the styling of the custom status.

3. Update https://github.com/Expensify/App/blob/d916effd3ffb2f29adcd3cfad1e314f210a23efa/src/CONST.ts#L885 this line with the height (26) is tested. more on here https://github.com/Expensify/App/issues/29162#issuecomment-1754788318

What alternative solutions did you explore? (Optional)

NA

[ishpaul777]

Proposal

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

Fix style regressions caused by new Global Nav component

What is the root cause of that problem?

1. Sign in Button has a width of 82 px while the globalNavigation container has width of 70 px so the sign in button does not fit in container

2. Avatar with status on global nav is only cropped same root cause as above^, The width is more the the globalNavigation container.

3. Avatar is added in this PR so before there was no avatar, so this issue was not noticable.

Changes

1. We need to replace the sign in button for anonymous user to Expensify Logo. We can use the AnonymousUser Footer for showing the sign in banner on the sidebar, However we might not need report details to shown in so we will introduce and new prop for conditionally rendering the report details in the banner, We also need to make some changes in styling and add message icon. I am assuming we need the sign in footer for full screen Sidebarscreen only, on large screen i dont see a need for the banner as we already have one in report footer

2. We can make position style changes in AvatarWithOptionalStatus similar to what we have done with avatar with indicator but in my humble opinion the status is too small to be clickable on touchscreen and may result in unexpexpected navigation for user, curious on your thoughts on this? cc @hayata-suenaga

3. On desktop, We can incrase the padding for the headerGap but then the overall header margin will increase on desktop it looks kind of weird, I would recommend we use negative "-12"(or a value decided by design team) margin top for BaseSidebarScreen and use another headergap for BaseSidebarScreen with same color as global nav, and add margin top on global nav

if we just increase the margin top -

[hayata-suenaga]

@shawnborton this is the follow up issue for the PR that added the new Global Nav component

[hayata-suenaga]

We decided that an engineer from an expert agency is going to work on this issue.

Thank you for all contributors who shared detailed proposals here. We really appreciate the time you took to write these 🙇

[hayata-suenaga]

@0xmiroslav you don't have to review proposals as an expert agency is going to work on it. We gonna keep you assigned to this issue so that you can review the PR when it's open 🙇

[hayata-suenaga]

Added this issue to the OP.

[stitesExpensify]

@kosmydel @robertKozik I heard you guys are interested in fixing these issues?

[hayata-suenaga]

@kosmydel @robertKozik could you also take a look at this issue?

[ishpaul777]

hii @hayata-suenaga, Just noticed that backgorund for preview for attachments is also changed to darkgreen color AFAIK before global nav refactor it was highlighted green color.

[hayata-suenaga]

omg the style system of App is just too intertwined...

@kosmydel @robertKozik ⬆️

[hayata-suenaga]

@kosmydel @robertKozik could you check if your fix will handle this regression, too?

[robertKozik]

@hayata-suenaga, I believe we didn't include this regression because we didn't notice it before. However, we can easily add it to the PR to keep all the fixes in the same place.

Should we proceed with the fix inside our PR as in my opinion it would be better to keep all related fixes inside one PR?

[mallenexpensify]

Commenting cuz I'm BZ on

• https://github.com/Expensify/App/issues/29870

@robertKozik , I lean towards agreeing with you, to keep all related fixes inside one PR. ( I'm not an engineer though).

[hayata-suenaga]

Should we proceed with the fix inside our PR as in my opinion it would be better to keep all related fixes inside one PR?

Yes let's handle in your PR 👍

We can close this issue if no payment is necessary (@robertKozik is a SWM engineeer)

[hayata-suenaga]

The PR was closed with implementation

Payment summary

C+ Review payment for @situchan

[ishpaul777]

hii @hayata-suenaga the Original issue and this issue was reported on slack, i think i should be eligible for the reporting payment

[hayata-suenaga]

@situchan could you check the above comment ⬆️ ?

[situchan]

Maybe we can pay @ishpaul777 one-time bonus ($50) for all these kinds of reports as they are regressions from one PR, if closed issues also apply bonus. @zanyrenney will decide.

[melvin-bot[bot]]

This issue has not been updated in over 15 days. @zanyrenney, @robertKozik, @0xmiroslav, @hayata-suenaga, @situchan eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[hayata-suenaga]

@zanyrenney could you handle the payment according to this suggestion?

[melvin-bot[bot]]

Current assignee @zanyrenney is eligible for the Bug assigner, not assigning anyone new.

[melvin-bot[bot]]

Bug0 Triage Checklist (Main S/O)

• [ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• [ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• [ ] This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• [ ] This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• [ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[zanyrenney]

https://www.upwork.com/jobs/~01a3d3d2d9336a21ba @ishpaul777 have invited you to the job.

[ishpaul777]

@zanyrenney Thanks! I accepted the invite

[melvin-bot[bot]]

@zanyrenney, @robertKozik, @0xmiroslav, @hayata-suenaga, @situchan Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[hayata-suenaga]

if the payment has been completed, we can close this issue 🙇

[situchan]

@hayata-suenaga can you please remove Reviewing which prevents adding Overdue? This way, @zanyrenney will be bumped every few days

[hayata-suenaga]

we're waiting for the payment?

[hayata-suenaga]

Please handle the payment according to this suggestion please 🙇

[situchan]

I noticed that @zanyrenney's slack status indicates OOO until Wed

[zanyrenney]

payment summary - @ishpaul777 $50 for regression reports. @situchan $500 for C+ review

As per this comment, closing.

[zanyrenney]

Paid @situchan through bonus due to problem with hourly contract.

[zanyrenney]

Paid @situchan via a bonus as I messed up the orignal offer and made it hourly. Closing!