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.35k stars 2.78k forks source link

Use persistent bottom tabs in Settings to improve Workspace Editor UX #44242

Open shawnborton opened 3 months ago

shawnborton commented 3 months ago

We're currently displaying Workspace Settings in the equivalent of a full screen modal. This means you can't easily navigate between Inbox, Search, and Workspace Settings - each time you close the Workspace Editor, you need to navigate back to Settings > Workspaces > Launch the workspace editor.

Let's make it so that the bottom tab bar in Settings is persistent or sticky, in that even after you navigate one or two levels deeper, it remains:

https://github.com/Expensify/App/assets/2319350/6925987c-3b44-4ec8-a391-7d94a502c49d

CleanShot 2024-06-24 at 12 35 20@2x

Other notes for completeness:

Figma for this is here

cc @Expensify/design @mountiny

mountiny commented 3 months ago

Waiting for updates from SWM

mountiny commented 2 months ago

Asked for an update here

mountiny commented 2 months ago

Will follow up with the team

mountiny commented 2 months ago

Asked here https://expensify.slack.com/archives/C04878MDF34/p1720655134137259

WojtekBoman commented 2 months ago

Hi! Sorry this took a while, but there was a lot of navigation issues recently, and we needed some time to clean things up. We investigated this topic together with Adam. Here are our thoughts:

The main change that we need to introduce is the possibility to display BottomTab conditionally on every page in the app, not only within the BottomTabNavigator (the way it's currently done). This approach requires some deeper changes to the current navigation architecture and may cause issues.

The first visible issue is connected with displaying bottom tab component in several navigators. Currently, it's rendered once in the BottomTabNavigator without any conditions, but to solve this issue we'd need to display it depending on the page and screen width (its position would no longer be fixed). It may cause small flickering of the bottom tab during transition between screens as the bottom tab contains the floating action button which has very complex logic.

Another problem is the inconsistency of the animations. Currently, when we navigate between screens with the bottom tab, we don't display any animation, and when we navigate from the bottom tab to the central pane, we have a slide animation. Now central pane screen can have a bottom tab, so sometimes when navigating to the CentralPane we would show an animation and other times not. This is presented in the video below.

So to sum up: as I mentioned earlier, the display of BottomTab on any screen in the app is related to the refactor of the navigation architecture. In the current structure we use BottomTabNavigator to display the bottom bar, now it would no longer be the responsibility of this navigator, so we will have to develop a new approach to handle display of the bottom tab.

What do you think about these considerations? Would you like us to continue working on this?

https://github.com/Expensify/App/assets/47774969/65dfd68d-89f3-4052-ad3e-76c706857eeb

mountiny commented 2 months ago

@WojtekBoman Thanks for the write up, I still think we want to explore this path more.

I believe its common to have the bottom tab on more pages in the app. It kinda felt like our solution is a bit custom before and what we are proposing here is more in line with standard usage of bottom tab navigator

shawnborton commented 2 months ago

Totally agree that we want to explore this further, especially given that we think we will want to reuse this pattern for Domains as well.

Kicu commented 2 months ago

@shawnborton @mountiny we will continue with the research to bring some more specific info. That is mostly done by @adamgrzybowski and @WojtekBoman but I'm sometimes helping a bit.

For now a question to get a bit more specifics:

The more requirements we know right now - the better the solution will be, and navigation in this app really is quite complex πŸ˜… Every time we want to do some change we need to think about: mobile native apps, wide layout and how would that affect URL/route for browsers.

shawnborton commented 2 months ago

Right now I think we're really only imagining the Settings > Workspaces > Workspace Editor, as well as (soon to be) Settings > Domains > Domain Editor. That being said, I could totally see an argument where we might as well do this for all Settings pages to be consistent, but I don't know if that is really a requirement or not. cc @Expensify/design for any additional thoughts too.

dannymcclain commented 2 months ago

No additional thoughts from me, I agree with your summary!

JmillsExpensify commented 2 months ago

I don't think it's required for the other pages, like profile or security and such, but I do think we should do this for workspaces (and domains in the future). I don't think we need to do it in other places.

mountiny commented 2 months ago

@Kicu @adamgrzybowski Do you have any further questions to get you unblocked? Thanks!

Kicu commented 2 months ago

Hey, we synced with guys and discussed some more details.

First to re-iterate: The main difficulty in our navigation is that we support both narrow and wide layouts and we expect the navigation to be consistent between the two. That means having a lot of code handling screens on narrow + bigger devices and transitioning from one to the other. In order to introduce the bottom bar in more screens we will have to modify said logic.

We would like to ask if it's possible for the UI/UX people to give us 2-3 examples of apps where navigation and bottom bar behaves consistently and similar to what we would like to have in Expensify in future? (Some examples of desktop apps (wide layout) would also be nice, but these are harder to find.) Looking at such apps would help us find any possible edge cases and understand different navigation flows better.

We have tried to search for desktop+mobile apps ourselves, but couldn't find any desktop apps that behave similar to ours. I have checked Spotify, Slack and Notion - not the deepest search but at least these are popular.

Some insights:

To sum up, going back to this quote from @mountiny

[...] our solution is a bit custom before and what we are proposing here is more in line with standard usage of bottom tab navigator

I agree with this in regards to mobile apps, but when talking about mobile+wide layout things are not so clear cut πŸ˜‰

Now for something less serious, here are some first experiments that @WojtekBoman did for the bottom bar πŸ˜…

https://github.com/user-attachments/assets/3afad7c2-c4a0-4795-b23e-c9fd2a689c27

^ I know this is silly, but it shows exactly the kinds of edge cases that we are ironing out right now, to avoid issues later.

adamgrzybowski commented 2 months ago

I agree with this in regards to mobile apps, but when talking about mobile+wide layout things are not so clear cut πŸ˜‰

Yes, this is an important point. I'd say we have a custom solution for the bottom tabs anyway. This does not depend on the number of screens that do or do not have a bottom tab bar.

What is custom and new though is that we want to introduce a conditional bottom tab depending on the screen size.

dannymcclain commented 2 months ago

Happy to try to look around a bit, but FWIW I think it's going to be pretty hard to find any other apps doing what we're doing. The way we try to maintain consistency across platforms is really not normal, so I suspect it will be difficult to find examples to draw inspiration from. But I'll try to take a look regardless!

shawnborton commented 2 months ago

Yeah, that's my general sentiment too. But to clarify, even if we don't find a perfect example, we can still make progress on this, right?

mountiny commented 2 months ago

Yeah, I agree. The fact our app is trying to maintain consistency pretty hard means that there are not many examples like this. This makes it a bit tricky to handle, I think what could help is if the design team created some interactive mockups with various flows to catch all the edge cases and establish the patterns we want in this updated form of navigation. Would that help to get a better understanding of what the expected output is?

shawnborton commented 2 months ago

Happy to make more mockups and flows if we think the original screenshots/video from the first comment above won't suffice. It's hard to think of any "edge cases" right now since we really only want to apply this to the Workspaces/Domains page.

mountiny commented 2 months ago

Maybe we might want to try to mocking some flow where the user resizes the window from wide to narrow, takes some further actions, and resizes back to wide. I know most people does not do that but we need to make sure the app navigation does not break when this happens

shawnborton commented 2 months ago

Cool, happy to do that. Let's see what SWM says they need in terms of more mocks/flows as well.

adamgrzybowski commented 2 months ago

I talked with guys and we came up with an idea. The base of requirements sounds quite simple. By the base I mean:

1. These screens have a bottom tab bar:

2. These screens have a bottom tab bar but only on small screen

I am not sure if we want Profile, Members, and other screens from the Workspace settings to have it. Please verify what is expected.

3. If you are navigated a level or more deep within Settings, tapping on the Settings tab in the bottom tab bar should bring you back to the top level of Settings

The last one is a little vague:

4. We don't want to modify any of the existing native app back button behavior (this came up in Slack)

Instead of theorizing which use cases can break, we will prepare a POC of points 1 to 3 and won't modify any other navigation behavior. Then you will be able to play around with the POC and create a list of cases where the navigation feels off. We assume that there will be at least a few cases like that.

I think it will be much easier to talk about the next steps when everybody can test and see examples instead of drawing screens with some arrows and comments.

shawnborton commented 2 months ago
  1. These screens have a bottom tab bar but only on small screen I am not sure if we want Profile, Members, and other screens from the Workspace settings to have it. Please verify what is expected.

We just want this for Workspaces to start.

  1. If you are navigated a level or more deep within Settings, tapping on the Settings tab in the bottom tab bar should bring you back to the top level of Settings The last one is a little vague:

The idea is that if you are in Settings > Workspaces > My Workspace, and you tap on the Settings button in the bottom nav bar, it will bring you all the way back to the top of the hierarchy to Settings. Let me know if that makes sense. It's kind of like a common behavior you see in other apps where pressing the bottom tab again in a page that is active might scroll you to the top or something.

Love the idea of a POC, excited to play with it!

mountiny commented 1 month ago

@WojtekBoman @Kicu What is the latest on this one?

adamgrzybowski commented 1 month ago

Hey, @mountiny I posted an update here but I guess it will be better to communicate on this issue.

Currently, I am working on the POC

mountiny commented 1 month ago

Thanks for the promising update Adam

adamgrzybowski commented 1 month ago

πŸ‘‹ Hey guys! I thought it may be worth briefly describing the POC I'm currently working on πŸ› οΈ

Currently, our root navigator is the one handling the split view. The first route is always the BottomTabNavigator containing all sidebar screens.

This may be problematic if we want to navigate to the bottom tab screen from a central pane screen on a narrow layout. We have to pop all other screens from the root navigator to uncover the first route.

The idea is to create many smaller split navigators that we can push on the root. There will be a split navigator for each of the tabs:

Now we will be able to push one split navigator on another one and we don't have to pop anything

This and the conditionally rendered bottom tab should be a solution for our issue.

There will be other benefits from these changes πŸͺ¨ 🀾 πŸ¦β€β¬› πŸ¦β€β¬›

I know the last one may be especially important given how this issue is going.

Excited to work on this issue! Back to coding! πŸ‘¨β€πŸ’»

cc: @mountiny @WojtekBoman

mountiny commented 1 month ago

πŸͺ¨ 🀾 πŸ¦β€β¬› πŸ¦β€β¬›

That sounds very promising! Thanks for the write up

shawnborton commented 1 month ago

What's the latest on this one? cc @JmillsExpensify

adamgrzybowski commented 1 month ago

Hello everyone! An update from me.

As mentioned earlier, the goal we want to achieve requires changes in navigation architecture.

I know that persistent bottom tabs don't sound like a big feature so it should be quick to implement but please keep in mind that these changes are much deeper and will make navigation simpler and more reliable. It should also fix at least a few of the old problems that would be unsolvable otherwise.

Thanks in advance for your patience πŸ™‡ and back to the update:

In the POC I am working on I managed to replace BottomTabNavigator and central pane screens with many split navigators. Those are quite big changes but the app handled it pretty well.

Changes mentioned above require modifications in some places related to navigation

linkTo: This function has been a pain for quite a long time. I rewritten it to do 90% of its previous functionality with 4x less code. Simplicity should help us with keeping this part bug-free. Needs a bit more work but not much.

SearchPage: With the previous approach we had to have a separate search page for the bottom tab and central pane. It was tricky to handle and fast resizing could lead to bugs. Now we can do it with just one page. I would say it's 80% refactored.

Workspace switcher: Works more or less for the search page and the part for reports is still to be done.

getAdaptedSatet: This is mainly cleaning the code that won't be necessary anymore. But there will be some adjustments.

goBack: This function is a pain as well. Implementation of many SplitNavigators should finally allow us to refactor it.

Navigation.ts: Similarly to the getAdaptedState, mainly cleaning and some adjustments.

FYI: I am going to be ooo for two next weeks. I will have some time on Monday though and I will pass my work to @WojtekBoman.

cc: @mountiny @shawnborton

mountiny commented 1 month ago

Amazing, thanks for the update @adamgrzybowski

JmillsExpensify commented 1 month ago

Agreed, thanks a ton for working on this one. Great update!

mountiny commented 3 weeks ago

@WojtekBoman @Kicu Any updates for this week even if Adam was out?

Kicu commented 3 weeks ago

None from me, right now I'm mostly working with Search and if there are search-related nav issues.

WojtekBoman commented 3 weeks ago

Hey @mountiny, I recently started working further on POC, last week I was involved in topics related to OldDot Rules migration. Now I'm focused on connecting the new navigation logic with storing the policyID.

adamgrzybowski commented 2 weeks ago

I am back from ooo and I will continue my work on this issue together with @WojtekBoman

mountiny commented 2 weeks ago

Not overdue

adamgrzybowski commented 6 days ago

Hi everyone πŸ‘‹ An update from us.

Changes visible to the user πŸ‘οΈ

From visible changes, we implemented @shawnborton requirements regarding bottom tab behavior.

https://github.com/user-attachments/assets/467a8f14-25be-4d05-866a-7ff9bbd7b646

https://github.com/user-attachments/assets/141e376c-48bf-4842-9976-844dbe062729

Besides we found two regressions that we also fixed.

Regression 1: Policy should be changed to all if the chat doesn't belong to it.

Before:

https://github.com/user-attachments/assets/4d58e9c0-0ec3-4cce-aef5-5e322fc2fd7f

After:

https://github.com/user-attachments/assets/6c705e55-09c5-4a44-b678-355d6c4a7b43

Regression 2: Some really weird behavior of search when resizing.

Before:

https://github.com/user-attachments/assets/5d7b1d3c-d930-40e1-bd5f-4b7166473d62

After:

https://github.com/user-attachments/assets/9102f1b8-b5f5-460b-891b-895382c192dc

Changes under the hood 🫣

However, what changed the most was the navigation structure. We know that the current structure is prone to something I would call the butterfly effect. Small changes in one place can cause regressions in others. My guess is that's how the regressions above were introduced.

That's why we are working on simplifying and bulletproofing navigation.

Here you can see an example of the old linkTo file that grew over time and was the root of many bugs.

image

And here's the new one.

image

We believe this approach can help us catch bugs that have not yet been reported and fit well with ongoing efforts focused on quality.

Testing

There are still things we need to adjust, but we think it's a good time to start testing the new navigators. We are removing a lot of complex code so we are expecting some regressions that we need to catch and fix.

Strategy

We talked about testing strategy and we want to utilize two ideas.

The first one As in previous iterations, navigate around the app to see if the app feels right. We can already start this one.

The second one Prepare more structured testing instructions.

We did notice that a lot of information about how navigation should work and what are the requirements is scattered somewhere over slack convos or in the closed issues.

We want to prepare a list of the testing steps covering all possible use cases for navigation separated into sections e.g. bottom tabs, go back.

This will be our list of requirements and testing steps at the same time. The idea is to extract them from already solved navigation issues.

Please let us know what you think about this approach and if you have an idea of what would be a good place to store the testing instructions for the navigation. We were thinking about the navigation documentation in the repo. Thanks!

cc: @mountiny @WojtekBoman

P.S. At some point we switched to a different branch so here's the new draft PR https://github.com/Expensify/App/pull/49539

dannymcclain commented 6 days ago

That is so sick πŸ₯Ή. This is coming along really well (at least from my perspective!)

Regression 1: Policy should be changed to all if the chat doesn't belong to it.

For this one, I think I was under the impression that if you filtered by a workspace, we'd only surface results that were in that workspace. Am I wrong about that? Can a smart person weigh in there? (If I'm wrong about that assumption then I think the fix for that makes total sense.)

mountiny commented 5 days ago

@dannymcclain i dont think so, at least not in the current state of the search we do not consider what workspace you have selected

So that would be a new feature afaik and the above mentioned expected behaviour is correct

@adamgrzybowski @WojtekBoman great job on this one, i think its looking great and having the navigation structure simplified is a welcomed bonus. Regarding the test steps, is that something you want to handle compose or you would like us to create? There are some testing steps already from the navigation refactor doc, they should be in the doc and we can base off them. Asking as it was not clear to me who is supposed to work on that πŸ˜‚

mountiny commented 5 days ago

I will try to trigger a build on the PR and start testing

trjExpensify commented 3 days ago

@dannymcclain i dont think so, at least not in the current state of the search we do not consider what workspace you have selected

The workspace switcher should inform the search page, like it does the inbox page. CC: @luacmartins

mountiny commented 3 days ago

Good to know, thanks for chiming in

dannymcclain commented 3 days ago

@dannymcclain i dont think so, at least not in the current state of the search we do not consider what workspace you have selected

So that would be a new feature afaik and the above mentioned expected behaviour is correct

Ok cool, thanks!

The workspace switcher should inform the search page, like it does the inbox page.

Sorry I wasn't talking about the search page, I was talking about the "Find" LHPβ€”those are still two separate things right? πŸ˜…

adamgrzybowski commented 3 days ago

@mountiny I think you can play around with the app to find anything that looks weird. We tried this approach before and you helped us a lot πŸ˜„ @WojtekBoman and I might miss some obvious bugs after looking at the new branch for so long.

For the structured testing steps I think we will create them and consult with you if they describe your requirements properly. That may help us find any miscommunication about how things should work.

After confirming we will add them to the docs.

BTW About the chat finder @dannymcclain I think we came to a conclusion long ago in some conversation that it would make more sense to allow the user to navigate to a chat outside of their workspace and switch to the "ALL" workspace than hide these chats and block the user. But I am not 100% πŸ˜„ Sounds like a perfect example of why we want to write down all these use cases in one place.

mountiny commented 3 days ago

I think we came to a conclusion long ago in some conversation that it would make more sense to allow the user to navigate to a chat outside of their workspace and switch to the "ALL" workspace than hide these chats and block the user.

I believe that is correct, at least that is how we designed this before.

Sounds great, I am using the web app now and it is working well so far!

adamgrzybowski commented 3 days ago

Cool! In that case, we will continue adjusting the old code to the new structure and will send you the first batch of testing steps/requirements to confirm soon.