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-10-04][$250] [Search v2.4] [App] Extend the 🔍icon and search router to all pages by adding “Search” #49122

Closed
luacmartins opened this issue Sep 12, 2024 · 18 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Sep 12, 2024

Implement this section of the doc

cc @Kicu @289Adam289 @Guccio163 @SzymczakJ

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021834649747497603669
  • Upwork Job ID: 1834649747497603669
  • Last Price Increase: 2024-09-13
  • Automatic offers:
    • ikevin127 | Reviewer | 104036646
Issue OwnerCurrent Issue Owner: @sakluger
@luacmartins luacmartins added Daily KSv2 NewFeature Something to build that is a new item. labels Sep 12, 2024
@luacmartins luacmartins moved this to Release 3: Fall 2024 (Nov) in [#whatsnext] #expense Sep 12, 2024
Copy link

melvin-bot bot commented Sep 12, 2024

Triggered auto assignment to @sakluger (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

@SzymczakJ
Copy link
Contributor

Hey! I’m Jakub Szymczak from Software Mansion, an expert agency, and I’d like to work on this issue!

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label Sep 13, 2024
@melvin-bot melvin-bot bot changed the title [Search v2.4] [App] Extend the 🔍icon and search router to all pages by adding “Search” [$250] [Search v2.4] [App] Extend the 🔍icon and search router to all pages by adding “Search” Sep 13, 2024
Copy link

melvin-bot bot commented Sep 13, 2024

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

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

melvin-bot bot commented Sep 13, 2024

Current assignee @ikevin127 is eligible for the External assigner, not assigning anyone new.

@luacmartins luacmartins removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 13, 2024
@melvin-bot melvin-bot bot added the Overdue label Sep 16, 2024
Copy link

melvin-bot bot commented Sep 16, 2024

@sakluger, @luacmartins, @ikevin127, @SzymczakJ Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@sakluger
Copy link
Contributor

@luacmartins Just confirming that this should be daily?

@melvin-bot melvin-bot bot removed the Overdue label Sep 16, 2024
@luacmartins
Copy link
Contributor Author

We can move it to weekly while we work on the first few issues to unblock this one.

@Kicu
Copy link
Contributor

Kicu commented Sep 19, 2024

I will start working on this task instead of @SzymczakJ, please reassign us

@luacmartins luacmartins assigned Kicu and unassigned SzymczakJ Sep 19, 2024
Copy link

melvin-bot bot commented Sep 19, 2024

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

@luacmartins
Copy link
Contributor Author

I believe this was deployed to production last week. @Kicu are you still working on anything else related to this issue?

@Kicu
Copy link
Contributor

Kicu commented Oct 4, 2024

Nope I don't think so, I'm only working on #49123 now

@luacmartins luacmartins changed the title [$250] [Search v2.4] [App] Extend the 🔍icon and search router to all pages by adding “Search” [HOLD for payment 2024-10-04][$250] [Search v2.4] [App] Extend the 🔍icon and search router to all pages by adding “Search” Oct 4, 2024
@luacmartins luacmartins added Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2 and removed Reviewing Has a PR in review Weekly KSv2 labels Oct 4, 2024
@luacmartins luacmartins moved this from Release 3: Fall 2024 (Nov) to Done in [#whatsnext] #expense Oct 4, 2024
@sakluger
Copy link
Contributor

sakluger commented Oct 4, 2024

Summarizing payment on this issue:

Contributor: @Kicu - no payment required
Contributor+: @ikevin127 $250, paid via Upwork

@sakluger sakluger closed this as completed Oct 4, 2024
Copy link

melvin-bot bot commented Oct 17, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@ntdiary
Copy link
Contributor

ntdiary commented Nov 1, 2024

Hi, @luacmartins, coming from #51192.
It seems that this issue and issue #49116 allow us to open a report using Search > Recent Chats on many pages?
Currently, this feature has some impact on navigation. I'm curious if the doc offers any guidance or suggestions on this? :)
e.g.,

  1. When clicking the back button in the report page, the LHN briefly appears (Or event when swiping the report page, we can't return to the Search page).
  2. Open a report on the Profile page, then swipe twice, it returns to the LHN instead of the Settings page.
test2.mp4

@luacmartins
Copy link
Contributor Author

@ntdiary I don't think we covered that in the doc and it's something we should address. @Kicu for your thoughts.

@Kicu
Copy link
Contributor

Kicu commented Nov 4, 2024

Hey, so first of all we treat this SearchRouter as a modal, so it's not a full fledged page (for example on web you will not see a dedicated URL for it).
So in theory this should not have much impact on navigation per se. It just appears when you open it - on top of whatever page was there, and then disappears after your interaction with it ends.

So if you open SearchRouter on Page X, then using either the back button or swipe should bring you back to Page X - I think this is the most natural transition.
So both 1 and 2 from @ntdiary comment sound like bugs that should be fixed. I think 🤔

Side note: calling @adamgrzybowski and @WojtekBoman who are working on navigation refactor, so it's possible this would be fixed on their PR, because the internal logic of screens, linkTo and back buttons will be changed after they are done.

On solution would be to wait, but I cannot guarantee how long this will take.
Not sure if that helps you a lot? 😓

@ntdiary
Copy link
Contributor

ntdiary commented Nov 4, 2024

Hey, so first of all we treat this SearchRouter as a modal, so it's not a full fledged page (for example on web you will not see a dedicated URL for it). So in theory this should not have much impact on navigation per se. It just appears when you open it - on top of whatever page was there, and then disappears after your interaction with it ends.

So if you open SearchRouter on Page X, then using either the back button or swipe should bring you back to Page X - I think this is the most natural transition. So both 1 and 2 from @ntdiary comment sound like bugs that should be fixed. I think 🤔

@Kicu, yeah, the navigation for SearchRouter modal itself works as expected. I think it seems to allow us to open a Report page from some other pages(e.g., Search page, Settings/Profile page), and at the same time, due to the logic within linkTo

if (topmostBottomTabRoute && (topmostBottomTabRoute.name !== matchingBottomTabRoute.name || isNewPolicyID || isOpeningSearch)) {
root.dispatch({
type: CONST.NAVIGATION.ACTION_TYPE.PUSH,
payload: matchingBottomTabRoute,
});
}

app will push a Home screen (i.e., the LHN) into the BottomTabNavigator. So when we swipe to close the Report page, we end up seeing the LHN instead of the expected Search page. 😂

Although we can continue to use the beforeRemoveReportOpenedFromSearchRHP listener to remove the LHN when closing the Report, I don’t think it can resolve the LHN flickering effectively, so it's not very ideal. My initial guess is that a better solution might require changing the above push logic, so it might be better to take care of this case during the navigation refactor. 😄

@Kicu
Copy link
Contributor

Kicu commented Nov 5, 2024

@ntdiary sounds good to me.

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 Daily KSv2 External Added to denote the issue can be worked on by a contributor NewFeature Something to build that is a new item.
Projects
Archived in project
Development

No branches or pull requests

6 participants