Skip to content
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

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

Closed
4 of 6 tasks
izarutskaya opened this issue Jul 30, 2024 · 20 comments
Closed
4 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2 Reviewing Has a PR in review

Comments

@izarutskaya
Copy link

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Bug6557060_1722324180554.ScreenRecording_07-30-2024_15-18-34_1.mp4

Bug6557060_1722324180584!production

View all open jobs on GitHub

@izarutskaya izarutskaya added DeployBlockerCash This issue or pull request should block deployment Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. DeployBlocker Indicates it should block deploying the API labels Jul 30, 2024
Copy link

melvin-bot bot commented Jul 30, 2024

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.

Copy link

melvin-bot bot commented Jul 30, 2024

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

@github-actions github-actions bot added Engineering Hourly KSv2 and removed Daily KSv2 labels Jul 30, 2024
Copy link
Contributor

👋 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
Copy link
Author

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

@nyomanjyotisa
Copy link
Contributor

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 #45996

Justify content set to flex-start if isSmallScreenWidth true

emptyStateForegroundStyles={isSmallScreenWidth && {justifyContent: 'flex-start'}}

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
Copy link
Contributor

@DylanDylann @koko57 Was this intentional ?

@koko57
Copy link
Contributor

koko57 commented Jul 30, 2024

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

@OfstadC OfstadC added Bug Something is broken. Auto assigns a BugZero manager. and removed Bug Something is broken. Auto assigns a BugZero manager. labels Jul 30, 2024
@OfstadC OfstadC removed their assignment Jul 30, 2024
Copy link

melvin-bot bot commented Jul 30, 2024

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
Copy link
Contributor

OfstadC commented Jul 30, 2024

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
Copy link
Contributor

koko57 commented Jul 30, 2024

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

@roryabraham roryabraham added Daily KSv2 and removed DeployBlocker Indicates it should block deploying the API DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jul 30, 2024
@koko57
Copy link
Contributor

koko57 commented Jul 31, 2024

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

@melvin-bot melvin-bot bot added the Overdue label Aug 1, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 2, 2024

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 #45996 - previously the modal had additional styles similar to the ones suggested in the previous Proposal

emptyStateContentStyles={isSmallScreenWidth ? {top: -BUTTON_HEIGHT} : undefined}

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:
    <View style={[{height: windowHeight - headerHeight}, styles.mt5]}>
  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)

Copy link
Contributor

github-actions bot commented Aug 2, 2024

true

Copy link

melvin-bot bot commented Aug 2, 2024

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

@DylanDylann
Copy link
Contributor

DylanDylann commented Aug 5, 2024

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

@DylanDylann
Copy link
Contributor

@anmurali Please assign me and @koko57 to this issue

@melvin-bot melvin-bot bot removed the Overdue label Aug 6, 2024
@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Aug 6, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 6, 2024

@DylanDylann PR ready for review #46860

@mountiny mountiny self-assigned this Aug 8, 2024
@melvin-bot melvin-bot bot removed the Weekly KSv2 label Aug 29, 2024
Copy link

melvin-bot bot commented Aug 29, 2024

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!

@melvin-bot melvin-bot bot added the Monthly KSv2 label Aug 29, 2024
@koko57
Copy link
Contributor

koko57 commented Aug 30, 2024

@mountiny I think we can close it

@mountiny
Copy link
Contributor

mountiny commented Sep 2, 2024

Thanks!

@mountiny mountiny closed this as completed Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. Engineering Monthly KSv2 Reviewing Has a PR in review
Projects
None yet
Development

No branches or pull requests

10 participants