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.03k stars 2.54k forks source link

[Held PR #42248] [Search v1] Date and Merchant value text isn't line with each other in the row #42269

Open izarutskaya opened 2 weeks ago

izarutskaya commented 2 weeks 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: 1.4.74.1 Reproducible in staging?: Y Reproducible in production?: Unable to check Found when validating PR : https://github.com/Expensify/App/pull/41672 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition: Have at least one submitted expense.

  1. Log in with an expensifail account
  2. Navigate to Account settings - Troubleshoot - New Search Page

Expected Result:

The whole row should be in line.

Actual Result:

Date and Mearchant value text isn't line with each other in the row.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Bug6482388_1715838047565!Capture

Screenshot 2024-05-16 at 17 53 52

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @Kicu
melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @zanyrenney (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 2 weeks ago

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

  1. If you find which PR caused the issue/bug, you can reassign it to the person responsible for it.
    • If the author is OOO or won’t get online before the daily deploy is due, you are responsible for finding the best fix/path forward. Don’t hesitate to ask for help!
  2. Try to reproduce the issue, if the bug is on production, remove the DeployBlocker label but stay assigned to fix it (or find out which PR broke it to get help from the author).
    • You can adjust the urgency of the issue to better represent the gravity of the bug.
    • If the issue is super low priority, feel free to un-assign yourself.
    • Be careful with PHP warnings, sometimes it is more complex than just adding a null coalescing operator as they might be uncovering some bigger bug.
    • If it was a one-off issue that requires no action (for example, Bedrock was down or it is a duplicated issue), you can close it.

Remember rule #2: Never un-assign yourself from a real DeployBlocker unless you are 100% sure someone else is assigned and will take care of it.

github-actions[bot] commented 2 weeks 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 2 weeks ago

@zanyrenney I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors.

Beamanator commented 2 weeks ago

@Expensify/design this looks like a bug, yeah? I think we can call this NAB cuz it's not very noticeable, thoughts?

trjExpensify commented 2 weeks ago

You can only access the search page right now via the troubleshooting menu (or knowing the URL directly) until we add the bottom tab for Search to release it. So definitely not a deploy blocker. 👍 CC: @luacmartins

shawnborton commented 2 weeks ago

Two things for you to follow up on @luacmartins:

The line height of any font/text that is at a size of 13px should be using 16px. Right now it's 18px for some reason?

Then if we take the min-height off of the value, it should vertically center itself just fine. CleanShot 2024-05-16 at 09 05 57@2x

So what we might want to consider is putting the min-height of 20px on the flex wrapper that wraps the value, and then make sure everything is vertically centered from there. Let me know if that makes sense!

Kicu commented 2 weeks ago

Hey I will take care of this because I'm actively working on Search components and its cheaper to just add this small fix as part of another PR, than to create a whole new PR just for this.

thanks @shawnborton for pointing out the font sizes, I'll straighten this out.

shawnborton commented 2 weeks ago

Wonderful - thanks!

Kicu commented 1 week ago

@shawnborton inside this PR I have updated text aligning: https://github.com/Expensify/App/pull/42248

You can check the screen or video, or run my branch if thats something you can do.

Not every font is the same size - for example names are smaller (I believe this is on purpose) but I expect things to look aligned/centered. Please tell me if this looks ok to you.

shawnborton commented 1 week ago

Sounds good, can you actually leave a similar comment on that PR too so we know to look for it during review? I'll spin up a test build on that PR as well. Thanks!

trjExpensify commented 1 week ago

Just to confirm, the PR for Add sorting will take care of this issue, right?

shawnborton commented 1 week ago

Yup!