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

[HOLD for payment 2024-08-01] [$50] Search icon has no safe area around and its hard to tap #45198

Closed
4 of 6 tasks
m-natarajan opened this issue Jul 10, 2024 · 40 comments
Closed
4 of 6 tasks
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 Jul 10, 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: 9.0.6-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: @mountiny
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1720613707699899

Action Performed:

  1. Launch the app
  2. Tap the Search icon for the chat selector

Expected Result:

The search icon tappable should be around the button anywhere

Actual Result:

Search icon seems so have super small safe area around, you really need to tap exactly on the search icon to open

Workaround:

Can the user still use Expensify without this being fixed? Have you informed them of the workaround?

Platforms:

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

Screenshots/Videos

image (12)

Add any screenshot/video evidence

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @anmurali
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~011f8d2de869135cc4
  • Upwork Job ID: 1811159428371033529
  • Last Price Increase: 2024-07-10
  • Automatic offers:
    • rayane-djouah | Contributor | 103071138
@m-natarajan m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 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.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jul 10, 2024

Proposal

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

Search icon seems so have super small safe area around, you really need to tap exactly on the search icon to open

What is the root cause of that problem?

No fixed height and width is assigned to PressableWithoutFeedback container.

<PressableWithoutFeedback
accessibilityLabel={translate('sidebarScreen.buttonFind')}
style={[styles.flexRow, styles.mr2]}
onPress={Session.checkIfActionIsAllowed(() => Navigation.navigate(ROUTES.CHAT_FINDER))}
>
<Icon
src={Expensicons.MagnifyingGlass}
fill={theme.icon}
/>
</PressableWithoutFeedback>

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

Add styles.touchableButtonImage to styles object and remove styles.mr2. We might also need to align the icon a bit by adding some left/right margin.

We can also add role={CONST.ROLE.BUTTON} if needed.

What alternative solutions did you explore? (Optional)

Result

search_button_fix.mp4

@Krishna2323
Copy link
Contributor

Proposal updated

@neonbhai
Copy link
Contributor

Proposal

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

Search icon seems so have super small safe area around, you really need to tap exactly on the search icon to open

What is the root cause of that problem?

We don't have any padding around the Search icon, making the hitbox small.

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

We will add styles.p1 to the button styles here, so the hitbox is bigger.

<PressableWithoutFeedback
accessibilityLabel={translate('sidebarScreen.buttonFind')}
style={[styles.flexRow, styles.mr2]}
onPress={Session.checkIfActionIsAllowed(() => Navigation.navigate(ROUTES.CHAT_FINDER))}
>

style={[styles.flexRow, styles.mr2, styles.p1]}

Result

Before Hitbox in red. Area is small and easy to miss Screenshot 2024-07-11 at 12 38 27 AM
After Hitbox in red. Added breathing space Screenshot 2024-07-11 at 12 36 59 AM

Without hitbox highlight:

@mountiny mountiny changed the title Search icon seems so have super small safe area around, you really need to tap exactly on the search icon to open Search icon has no safe area around and its hard to tap Jul 10, 2024
@mountiny mountiny self-assigned this Jul 10, 2024
@mountiny
Copy link
Contributor

I will make this external for $50 as its super easy

@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Jul 10, 2024
@melvin-bot melvin-bot bot changed the title Search icon has no safe area around and its hard to tap [$250] Search icon has no safe area around and its hard to tap Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

Job added to Upwork: https://www.upwork.com/jobs/~011f8d2de869135cc4

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

melvin-bot bot commented Jul 10, 2024

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

@mountiny mountiny changed the title [$250] Search icon has no safe area around and its hard to tap [$50] Search icon has no safe area around and its hard to tap Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

Upwork job price has been updated to $50

@mountiny
Copy link
Contributor

@Ollyws all yours now

@Krishna2323
Copy link
Contributor

@mountiny, I agree it's an easy one, but it still required time to determine the appropriate styling. I propose using the same styling as the back button, which would maintain consistency and aligns with how we style other icons. Additionally, we invest fair amount of time recording videos for each platform. Therefore, I'd like to request an increase to $125. That's my perspective, and I'm open to hearing your thoughts.

@rayane-djouah
Copy link
Contributor

@rayane-djouah
Copy link
Contributor

rayane-djouah commented Jul 10, 2024

@Krishna2323's proposal looks good to me. I think using the same styling as the back button sounds good (we use styles.touchableButtonImage in a lot of similar icons).

🎀👀🎀 C+ reviewed

Copy link

melvin-bot bot commented Jul 10, 2024

Current assignee @mountiny is eligible for the choreEngineerContributorManagement assigner, not assigning anyone new.

@Ollyws Ollyws removed their assignment Jul 10, 2024
@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Jul 10, 2024
Copy link

melvin-bot bot commented Jul 10, 2024

📣 @rayane-djouah 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

@mountiny
Copy link
Contributor

@Krishna2323 lets stick with $50 for now and see if there will be any reason to bump to $125.

@mountiny
Copy link
Contributor

44x44 is the apple standard minimum for the button so lets try to implement that please @Krishna2323 thanks!

@melvin-bot melvin-bot bot added the Awaiting Payment Auto-added when associated PR is deployed to production label Jul 25, 2024
@melvin-bot melvin-bot bot changed the title [$50] Search icon has no safe area around and its hard to tap [HOLD for payment 2024-08-01] [$50] Search icon has no safe area around and its hard to tap Jul 25, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Jul 25, 2024
Copy link

melvin-bot bot commented Jul 25, 2024

Reviewing label has been removed, please complete the "BugZero Checklist".

Copy link

melvin-bot bot commented Jul 25, 2024

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.11-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-08-01. 🎊

For reference, here are some details about the assignees on this issue:

Copy link

melvin-bot bot commented Jul 25, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@rayane-djouah] The PR that introduced the bug has been identified. Link to the PR:
  • [@rayane-djouah] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
  • [@rayane-djouah] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
  • [@rayane-djouah] Determine if we should create a regression test for this bug.
  • [@rayane-djouah] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@anmurali] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Jul 31, 2024
@rayane-djouah
Copy link
Contributor

  • The PR that introduced the bug has been identified. Link to the PR: Ideal nav v2 workspaces #37421
  • The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: https://github.com/Expensify/App/pull/37421/files#r1702108483
  • A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: N/A
  • Determine if we should create a regression test for this bug. Yes
  • If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Proposal

  1. Launch the app
  2. Tap around the Search icon for the chat selector
  3. Verify Search icon has tappable area round it

Do we agree 👍 or 👎

@rayane-djouah
Copy link
Contributor

cc @anmurali

@melvin-bot melvin-bot bot added the Overdue label Aug 2, 2024
Copy link

melvin-bot bot commented Aug 5, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...

@anmurali
Copy link

anmurali commented Aug 6, 2024

@rayane-djouah is paid
@Krishna2323 can you pls link me your Upwork profile?

@melvin-bot melvin-bot bot removed the Overdue label Aug 6, 2024
@Krishna2323
Copy link
Contributor

@anmurali, Upwork profile.

@melvin-bot melvin-bot bot added the Overdue label Aug 8, 2024
Copy link

melvin-bot bot commented Aug 9, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 Whoops! This issue is 2 days overdue. Let's get this updated quick!

Copy link

melvin-bot bot commented Aug 13, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

@anmurali
Copy link

@Krishna2323 offer is here

@melvin-bot melvin-bot bot removed the Overdue label Aug 14, 2024
@Krishna2323
Copy link
Contributor

Accepted, thanks.

@melvin-bot melvin-bot bot added the Overdue label Aug 16, 2024
Copy link

melvin-bot bot commented Aug 19, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 Eep! 4 days overdue now. Issues have feelings too...

@rayane-djouah
Copy link
Contributor

@anmurali - What's the latest here?

Copy link

melvin-bot bot commented Aug 21, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 Still overdue 6 days?! Let's take care of this!

Copy link

melvin-bot bot commented Aug 23, 2024

@anmurali, @mountiny, @rayane-djouah, @Krishna2323 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

@anmurali
Copy link

This was paid a while back. Closing!

@melvin-bot melvin-bot bot removed the Overdue label Aug 26, 2024
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
None yet
Development

No branches or pull requests

7 participants