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

[$125] "No results found" text is not aligned to left edge #37984

Closed
2 of 6 tasks
m-natarajan opened this issue Mar 8, 2024 · 69 comments
Closed
2 of 6 tasks

[$125] "No results found" text is not aligned to left edge #37984

m-natarajan opened this issue Mar 8, 2024 · 69 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@m-natarajan
Copy link

m-natarajan commented Mar 8, 2024

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.49-0
Reproducible in staging?: y
Reproducible in production?: y
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: https://expensify.slack.com/archives/C049HHMV9SM/p1709846865518079

Action Performed:

  1. Open app
  2. Click on search
  3. Type some random text which will give "No results found" message
  4. Verify the alignment with left edge

Expected Result:

Should be aligned with left edge

Actual Result:

Not aligned with left edge

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

Add any screenshot/video evidence

CleanShot 2024-03-07 at 16 27 53@2x

Screen Shot 2024-03-08 at 10 43 18 AM

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~019fd4e434eac176f9
  • Upwork Job ID: 1766288950642847744
  • Last Price Increase: 2024-03-16
  • Automatic offers:
    • brandonhenry | Contributor | 0
Issue OwnerCurrent Issue Owner: @greg-schroeder
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Mar 8, 2024
Copy link

melvin-bot bot commented Mar 8, 2024

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

@apeyada
Copy link
Contributor

apeyada commented Mar 8, 2024

Proposal

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

"No results found" is not aligned to left edge

What is the root cause of that problem?

input view has 16px horizontal padding but header message has 20px horizontal padding

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

set 16px everywhere

<View style={[styles.ph5, styles.pb5]}>

update this style to [styles.ph4, styles.pb5]

What alternative solutions did you explore? (Optional)

set 20px everywhere

<View style={[styles.ph4, styles.pb3]}>

update this style to [styles.ph5, styles.pb3]

@shawnborton
Copy link
Contributor

@greg-schroeder just a note that we want to make the bounty on this one super small given that it's such a tiny change.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 8, 2024

Proposal

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

"No results found" text is not aligned to left edge

What is the root cause of that problem?

we set the style of the header message to the following which is not consistent with the search field titie:

Header message:

<View style={[styles.ph5, styles.pb5]}>
<Text style={[styles.textLabel, styles.colorMuted]}>{headerMessage}</Text>

Search box:

<View style={[styles.ph4, styles.pb3]}>

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

We need to check first if the text in header message is : common.noResultsFound and if so then set the style according to the search input header.

Note: we should not directly change the view style of the headerMessage as this will cause a regression when we actually have search results and it will not align properly

What alternative solutions did you explore? (Optional)

N/A

@greg-schroeder greg-schroeder changed the title "No results found" text is not aligned to left edge [$125] "No results found" text is not aligned to left edge Mar 9, 2024
Copy link

melvin-bot bot commented Mar 9, 2024

⚠️ Could not update price automatically because there is no linked Upwork Job ID. The BZ team member will need to update the price manually in Upwork.

@greg-schroeder greg-schroeder added the External Added to denote the issue can be worked on by a contributor label Mar 9, 2024
Copy link

melvin-bot bot commented Mar 9, 2024

Job added to Upwork: https://www.upwork.com/jobs/~019fd4e434eac176f9

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Mar 9, 2024
Copy link

melvin-bot bot commented Mar 9, 2024

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

@brandonhenry
Copy link
Contributor

Proposal

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

The "No results found" text in the search page is not aligned with the left edge of the search input. We want the "No results found" text to have the same left padding as the search input for consistency.

What is the root cause of that problem?

The root cause is that the headerMessage prop passed to BaseSelectionList is being rendered with ph5 (padding horizontal 5) style, while the search input is rendered with ph4 style. This causes the misalignment.

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

I propose we modify BaseSelectionList to accept an optional headerMessageStyle prop. This prop would default to [styles.ph5, styles.pb5] to maintain the existing styling in most cases.

However, when rendering the search page, we can pass a custom headerMessageStyle prop to override the padding:

<SelectionList
    ...
    headerMessage={headerMessage}
    headerMessageStyle={headerMessage === translate('common.noResultsFound') 
      ? [styles.ph4, styles.pb5] 
      : undefined
    }
    ...
/>

This way, the "No results found" message will have ph4 padding to match the search input, but other uses of headerMessage will retain the original ph5 padding.

What alternative solutions did you explore? (Optional)

Another option would be to always use ph4 padding for headerMessage. However, this could potentially break the alignment/styling in other places where BaseSelectionList is used.

By making headerMessageStyle customizable, we can surgically fix the search page alignment without risking unintended side effects elsewhere in the app.

@melvin-bot melvin-bot bot added the Overdue label Mar 11, 2024
Copy link

melvin-bot bot commented Mar 12, 2024

@greg-schroeder, @allroundexperts Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@greg-schroeder
Copy link
Contributor

Proposal review

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Mar 13, 2024
Copy link

melvin-bot bot commented Mar 16, 2024

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

@greg-schroeder
Copy link
Contributor

Bump @allroundexperts

@melvin-bot melvin-bot bot removed the Overdue label Mar 17, 2024
@allroundexperts

This comment was marked as outdated.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 17, 2024

Note

@allroundexperts , No conflict with your chosen proposal but can i ask what did my proposal lack, I guess i proposed a similar solution:)
thanks

@allroundexperts
Copy link
Contributor

@allroundexperts , No conflict with your chosen proposal but can i ask what did my proposal lack, I guess i proposed a similar solution:)

thanks

From my understanding, your proposal was lacking where to put the changes.

@allgandalf
Copy link
Contributor

allgandalf commented Mar 17, 2024

ohh okay, understood, thanks for the explanation:) I thought i mentioned the baseselection list in the RCA

Still would leave it to the internal engineer to consider my proposal as i had shown the correct place in RCA

@melvin-bot melvin-bot bot added the Overdue label Mar 19, 2024
Copy link

melvin-bot bot commented Mar 20, 2024

@greg-schroeder, @allroundexperts Whoops! This issue is 2 days overdue. Let's get this updated quick!

@brandonhenry
Copy link
Contributor

Still awaiting assignment but can have this turned around by Saturday

@brandonhenry
Copy link
Contributor

@shawnborton can we get some more details? What did you see that on? I just checked on all devices and did not see what you shared in your pic 👀

@brandonhenry
Copy link
Contributor

Ahh I see. @shawnborton what you shared, seems to be related to the category page. This ticket, is specifically referencing the search results. So, you seem to have found a separate bug to this one

Cc. @greg-schroeder

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@grgia
Copy link
Contributor

grgia commented Apr 29, 2024

Is the root issue the same? Are we not using the same component for these two inputs?

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@shawnborton
Copy link
Contributor

Yeah, apologies if we didn't make it clear in the issue but ideally this is solved everywhere as to me, this feels like the same kind of component/UX.

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Apr 29, 2024
@brandonhenry
Copy link
Contributor

brandonhenry commented Apr 29, 2024

It seems similar and I can make the fix! I was instructed to only focus on the specific bug in an alternative PR so wasn't aware I needed to test "similar UX" components..

The fix for this, was applying styling specifically in the SearchPage component. Based on new info, the goal should be to locate all areas the search bar is used and add styling when results are not found. After further analysis, the headerMessage currently is used in the same way across all search types. That said, I have applied the simple additional fix to my solution to make this work across "no results found" error messages

PR: #41221

@shawnborton @grgia @allroundexperts

@greg-schroeder
Copy link
Contributor

PR remains in progress/review w/ @shawnborton

@shawnborton
Copy link
Contributor

The PR above looks good to me, but now we need a C+ to run through the checklist. Will comment on the PR too.

@greg-schroeder
Copy link
Contributor

PR looks all green, we should be ready to merge eh?

@greg-schroeder greg-schroeder changed the title [C+ Checklist Needs Completion] [$125] "No results found" text is not aligned to left edge [$125] "No results found" text is not aligned to left edge May 15, 2024
@brandonhenry
Copy link
Contributor

@greg-schroeder indeed, should be good to go. not sure what would hold it 👀
Cc. @allroundexperts @shawnborton

@shawnborton
Copy link
Contributor

Just merged, sorry about that!

@brandonhenry
Copy link
Contributor

no worries at all, i thought it was automatic 😄

@greg-schroeder
Copy link
Contributor

Merged. Awaiting deploy to staging -> prod

@brandonhenry
Copy link
Contributor

@greg-schroeder updates here? just saw this one lingering - no rush :D

@mallenexpensify mallenexpensify removed the Reviewing Has a PR in review label Jun 14, 2024
@mallenexpensify
Copy link
Contributor

Looks like this hit production 3 weeks ago

Looks like you were auto-paid @brandonhenry cuz you were hired on a milestone.
image

@greg-schroeder , I removed Reviewing, can you wrap this up? thx

@mallenexpensify mallenexpensify added Daily KSv2 and removed Weekly KSv2 labels Jun 14, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 17, 2024
@greg-schroeder
Copy link
Contributor

Ah, sorry folks, the automation didn't run, so my tracking didn't pick up payment pending

@melvin-bot melvin-bot bot removed the Overdue label Jun 17, 2024
@greg-schroeder
Copy link
Contributor

Payment summary:

Contributor: @brandonhenry - $125 - paid via upwork
C+: @allroundexperts - $125 - ND Manual Request

cc @JmillsExpensify

@JmillsExpensify
Copy link

$125 approved for @allroundexperts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Status: CRITICAL
Development

No branches or pull requests