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.33k stars 2.76k forks source link

Expensify Card - Empty state card is top centered instead of middle centered #46488

Closed izarutskaya closed 1 week ago

izarutskaya commented 1 month 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.14-2 Reproducible in staging?: Y Reproducible in production?: N Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Launch New Expensify app.
  2. Go to workspace settings.
  3. Go to Expensify Card.

Expected Result:

The empty state card will be middle centered (production behavior).

Actual Result:

Empty state card is top centered instead of middle centered.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/6f210635-f344-4d7a-9a20-cfdeaf973394

Bug6557060_1722324180584!production

View all open jobs on GitHub

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @youssef-lr (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 1 month ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
izarutskaya commented 1 month ago

We think this issue might be related to the #collect project.

nyomanjyotisa commented 1 month ago

Proposal

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

What is the root cause of that problem?

Coming from https://github.com/Expensify/App/pull/45996

Justify content set to flex-start if isSmallScreenWidth true https://github.com/Expensify/App/blob/75be218706600c42c433540bdac2889f84479428/src/pages/workspace/expensifyCard/EmptyCardView.tsx#L41

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

We should change that to the following to make it center like on production and with the new button margin

emptyStateForegroundStyles={isSmallScreenWidth && {top: -(BUTTON_HEIGHT + BUTTON_MARGIN)}}

image

What alternative solutions did you explore? (Optional)

allgandalf commented 1 month ago

@DylanDylann @koko57 Was this intentional ?

koko57 commented 1 month ago

Kind of. It was hard to center, bc we have a content below that appears after scrolling and on some devices the modal was cut off at the bottom (making the text hidden). I can work on this to fix it

melvin-bot[bot] commented 1 month 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.

OfstadC commented 1 month ago

Assigned to me while I was OoO - reassigning since this is time sensitive and i'm not back online fully until tomorrow . I can nab it back tomorrow if this ends up not being a blocker - or as urgent

koko57 commented 1 month ago

it should not be a blocker as it's behind the beta

koko57 commented 1 month ago

As I introduced the change and I am still working on the Workspace Feed project, can I be assigned to fix this issue?

cc @mountiny

koko57 commented 1 month ago

Proposal

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

The Empty state modal is not centered.

What is the root cause of that problem?

Changes from https://github.com/Expensify/App/pull/45996 - previously the modal had additional styles similar to the ones suggested in the previous Proposal

https://github.com/Expensify/App/blob/5430694a832c70b0541dff5d12242f5a204f00e5/src/pages/workspace/expensifyCard/EmptyCardView.tsx#L34

but they were insufficient in some cases, because on some devices the modal was cut off - that's why I introduced the "flex-start" change.

The real problem though was that the EmptyStateComponent wrapper's height was not calculated properly.

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

  1. Change mt5 to pt5 (margin to padding) in this line: https://github.com/Expensify/App/blob/5430694a832c70b0541dff5d12242f5a204f00e5/src/pages/workspace/expensifyCard/EmptyCardView.tsx#L26
  2. On iOS we additionally need to add SafeArea inset top to the equation for calculating header height, because it's not taken into account in windowHeight value (it's the same as screenHeight)
Screenshot 2024-08-01 at 20 16 16

this way we'll get the proper height for both iOS and Android (please ignore red line, the teal line is important here) Screenshot_1722535171 Screenshot_1722535227 Simulator Screenshot - iPhone 15 Pro - 2024-08-01 at 19 59 42 Simulator Screenshot - iPhone 15 Pro - 2024-08-01 at 19 59 33

and we don't see the skeletons after scrolling (which was the case on some devices earlier) but the disclaimer is appearing right away.

What alternative solutions did you explore? (Optional)

-

github-actions[bot] commented 1 month ago

true

melvin-bot[bot] commented 1 month ago

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

DylanDylann commented 1 month ago

@koko57 This belongs to our project. Please raise a quick PR for this

DylanDylann commented 1 month ago

@anmurali Please assign me and @koko57 to this issue

koko57 commented 1 month ago

@DylanDylann PR ready for review https://github.com/Expensify/App/pull/46860

melvin-bot[bot] commented 2 weeks ago

This issue has not been updated in over 15 days. @youssef-lr, @anmurali, @koko57, @mountiny, @DylanDylann 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!

koko57 commented 2 weeks ago

@mountiny I think we can close it

mountiny commented 1 week ago

Thanks!