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] [$250] [Search v1] Refactor RHP flow to contain navigation within itself #41020

Open
luacmartins opened this issue Apr 25, 2024 · 22 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Hot Pick Ready for an engineer to pick up and run with Overdue Weekly KSv2

Comments

@luacmartins
Copy link
Contributor

luacmartins commented Apr 25, 2024

Problem

We introduced reports in the RHP in this PR, however, interacting with other navigation actions, e.g. clicking on a task, makes the task open in the main pane which is jarring and unexpected.

Coming from #40570 (review), we need to update App to handle RHP navigation within the RHP. That means handling navigation events for mentions, rooms, any links, IOUs, images, etc

Solution

Let's discuss

cc @WojtekBoman @adamgrzybowski @Kicu this is not a priority now, but it's a polish item we'll need to work on once the core issues have been implemented.

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01a6d3b7c07ca93531
  • Upwork Job ID: 1790718951241498624
  • Last Price Increase: 2024-05-15
Issue OwnerCurrent Issue Owner: @rushatgabhane
@luacmartins luacmartins added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 25, 2024
@luacmartins luacmartins self-assigned this Apr 25, 2024
Copy link

melvin-bot bot commented Apr 25, 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.

Copy link

melvin-bot bot commented Apr 29, 2024

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

@luacmartins luacmartins added Weekly KSv2 and removed Daily KSv2 labels Apr 29, 2024
@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@luacmartins
Copy link
Contributor Author

Moving this to weekly for now while we work on the MVP for search.

@luacmartins
Copy link
Contributor Author

Still no updates here as we're focusing on the other v1.2 items at the moment.

@melvin-bot melvin-bot bot removed the Overdue label May 8, 2024
@trjExpensify
Copy link
Contributor

Going to move it into the summer along with the v1.2 items!

Copy link

melvin-bot bot commented May 9, 2024

@anmurali @luacmartins this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

Copy link

melvin-bot bot commented May 13, 2024

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

@adamgrzybowski
Copy link
Contributor

Hi guys! Two things I want to mention:

UX Perspective:
I wonder if putting all possible central panel flows into RHP gives value to the user. For people using desktops web version we will be using only a small part of the screen.
Don't you think displaying all these screens in two different contexts could also be confusing to the users?

Technical Perspective:
I spent a few minutes trying to think how time consuming the implementation might be, and I believe it will become quite complex. Some pitfalls:

  • We would need to replace all links sent in chat to reflect the RHP versions of these pages if we want them to navigate in RHP. It may be confusing for the user to press /r/123123 and see search/all/view/123123
  • On the central panel we have logic and optimizations to handle the same screen opened several times with different parameters (report screens). We don’t have that in RHP.
  • As usual, we would have to take a closer look at the browser history. It is easy to mess it up.

I suspect there may be plenty of similar problems.
I would advise to reserve some time for a research how difficult this will be first.

So the final questions are:

  • are we aware of the cost of implementation?
  • is there really a value in this?
  • is cost/value ratio satisfying enough to focus on this now.

@Kicu found a good example on GitHub where if are on you a project view you can quickly look into the PR using RHP, but any further navigation will open on full screen.

cc: @WojtekBoman

web.mov

@luacmartins
Copy link
Contributor Author

cc @trjExpensify @JmillsExpensify @shawnborton for opinions on the comment above

@melvin-bot melvin-bot bot removed the Overdue label May 14, 2024
@shawnborton
Copy link
Contributor

Hmm personally I think it's quite valuable to allow the RHP to navigate a couple levels deep. For instance, let's say you are viewing a report - you might want to drill down into an expense from there.

I can't speak to the technical hurdles of doing this, but I do think it's worth exploring further.

We did talk about the idea of having some kind of expand button that might allow you to make the RHP turn into a full page view somehow, so that's something to think about as well.

@luacmartins luacmartins added the External Added to denote the issue can be worked on by a contributor label May 15, 2024
@melvin-bot melvin-bot bot changed the title [Search v1] Refactor RHP flow to contain navigation within itself [$250] [Search v1] Refactor RHP flow to contain navigation within itself May 15, 2024
Copy link

melvin-bot bot commented May 15, 2024

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

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

melvin-bot bot commented May 15, 2024

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

@JmillsExpensify
Copy link

Hmm, interesting convo. I think one struggle I have is that I expect us to eventually move to a three-pane view, so I'm also hesitant to invest quite a bit in the RHP view.

Copy link

melvin-bot bot commented May 20, 2024

@anmurali, @luacmartins, @rushatgabhane Huh... This is 4 days overdue. Who can take care of this?

@melvin-bot melvin-bot bot added the Overdue label May 20, 2024
@JmillsExpensify
Copy link

Coming from Slack, I recommend that we effectively hold this issue, such that for the v1, we don't support broader navigation in the RHP. You can use it to open/see more report/expense details directly, but that's the extent of it.

@JmillsExpensify
Copy link

@anmurali I hope you don't mind that I take this issue over in conjunction with the on-going search implementation.

@rushatgabhane
Copy link
Member

not overdue

@JmillsExpensify JmillsExpensify changed the title [$250] [Search v1] Refactor RHP flow to contain navigation within itself [HOLD] [$250] [Search v1] Refactor RHP flow to contain navigation within itself May 21, 2024
@JmillsExpensify JmillsExpensify added Weekly KSv2 and removed Daily KSv2 labels May 21, 2024
@WojtekBoman
Copy link
Contributor

Hi! Today I started investigating how we can implement RHP flow in the Search module and I've found one case that I'd like to discuss. I'm wondering if we can just continue navigating in RHP when we navigate to the /r/:reportID path and we are on the report page displayed in RHP. But in this solution, we'll also remain in RHP when we click on the deeplink in the format /r/:reportID . Is this behaviour ok for you? @Expensify/design @JmillsExpensify @trjExpensify

Screen.Recording.2024-05-27.at.17.10.08.mov

@shawnborton
Copy link
Contributor

I think that would be okay for me, but just curious how you navigated to a #room in the RHP in the first place? Was that linked from an expense or expense report somehow?

@trjExpensify
Copy link
Contributor

Yeah, same. 👍

@adamgrzybowski
Copy link
Contributor

Hi! What is the status of this task? Do we have a clear idea what we want to implement? We have some capacity to work on it.

cc: @luacmartins @shawnborton @trjExpensify

@melvin-bot melvin-bot bot added the Overdue label Jun 7, 2024
@luacmartins
Copy link
Contributor Author

No updates yet. I think we'll have to start a broader discussion on this pattern before we can work on solutions.

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. External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors Hot Pick Ready for an engineer to pick up and run with Overdue Weekly KSv2
Projects
Status: Tracking issues
Development

No branches or pull requests

8 participants