Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.55k stars 2.89k forks source link

Inbox LHN wrapper is missing 12px of top padding #52198

Open m-natarajan opened 5 days ago

m-natarajan commented 5 days ago

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.59-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught on HybridApp, is this reproducible on New Expensify Standalone?: 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 (hyperlinked to channel name): ts_external_expensify_bugs

Action Performed:

  1. Go to staging.new.expensify.com
  2. Observe LHN padding

    Expected Result:

    12px padding for the wrapper that wraps LHN items

    Actual Result:

    Inbox doesn't have this same top padding.

    Workaround:

    Unknown

    Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

CleanShot 2024-11-07 at 10 04 39@2x

View all open jobs on GitHub

melvin-bot[bot] commented 5 days ago

Triggered auto assignment to @anmurali (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.

mkzie2 commented 5 days ago

Edited by proposal-police: This proposal was edited at 2024-11-08 03:07:59 UTC.

Proposal

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

Inbox doesn't have this same top padding.

What is the root cause of that problem?

We don't add padding or margin style to the wrap View of LHN items

https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/pages/home/sidebar/SidebarLinks.tsx#L90

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

We can add styles.mv3 or styles.pv3 if we want to add both margin/padding top and bottom

https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/pages/home/sidebar/SidebarLinks.tsx#L90

Or if we only want to add a margin/padding top, we can add styles.mt3 or styles.pt3 here

https://github.com/Expensify/App/blob/daf0addf17040ae0b0ad76e9705d20a7143b9a39/src/pages/home/sidebar/SidebarLinks.tsx#L90

What alternative solutions did you explore? (Optional)

mkzie2 commented 5 days ago

@shawnborton Actually, the padding is added to the inner component.

Screenshot 2024-11-07 at 23 20 55

The expectation we want here is to move this to the outside component like this, right?

Screenshot 2024-11-07 at 23 23 40
Themoonalsofall commented 5 days ago

Proposal

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

Inbox LHN wrapper is missing 12px of top padding

What is the root cause of that problem?

New style for inbox LHN

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

We can style Inbox LHN wrapper like we did with Search page and workspace

search workspace

https://github.com/Expensify/App/blob/2ef91b8b9d36f9534969d7bb0e3133b2a848e7a4/src/styles/index.ts#L1856

https://github.com/Expensify/App/blob/2ef91b8b9d36f9534969d7bb0e3133b2a848e7a4/src/pages/home/sidebar/SidebarLinks.tsx#L90

<View style={[styles.pRelative, styles.flex1, styles.mt3, styles.mh3]}>
result

What alternative solutions did you explore? (Optional)

More simply just need to add styles.mt3 or styles.pt3 to wrapper

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

mkzie2 commented 5 days ago

Updated proposal.

shawnborton commented 5 days ago

Yeah @Themoonalsofall's proposal is what I had in mind for this.

melvin-bot[bot] commented 1 day ago

@anmurali Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 1 day ago

Triggered auto assignment to @sonialiap (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.