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

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

Open
shawnborton opened this issue Jun 24, 2024 · 59 comments
Open
Assignees
Labels
Internal Requires API changes or must be handled by Expensify staff Weekly KSv2

Comments

@shawnborton
Copy link
Contributor

shawnborton commented Jun 24, 2024

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:

CleanShot.2024-06-17.at.10.17.43.mp4

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

Other notes for completeness:

  • We'll want to reshuffle the ordering of the LHN items for Settings so that Workspaces is higher up on the list and more discoverable (and always above the fold)
  • We don't want to modify any of the existing native app back button behavior (this came up in Slack)
  • 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

Figma for this is here

cc @Expensify/design @mountiny

@mountiny mountiny self-assigned this Jun 24, 2024
@mountiny mountiny added the Daily KSv2 label Jun 24, 2024
@melvin-bot melvin-bot bot added the Overdue label Jun 26, 2024
@mountiny
Copy link
Contributor

Waiting for updates from SWM

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Jun 26, 2024
@mountiny
Copy link
Contributor

mountiny commented Jul 2, 2024

Asked for an update here

@melvin-bot melvin-bot bot removed the Overdue label Jul 2, 2024
@mountiny mountiny added Weekly KSv2 and removed Daily KSv2 labels Jul 2, 2024
@melvin-bot melvin-bot bot added the Overdue label Jul 10, 2024
@mountiny
Copy link
Contributor

Will follow up with the team

@melvin-bot melvin-bot bot removed the Overdue label Jul 10, 2024
@mountiny
Copy link
Contributor

@WojtekBoman
Copy link
Contributor

WojtekBoman commented Jul 11, 2024

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?

Screen.Recording.2024-07-09.at.16.48.22.mov

@mountiny
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

Kicu commented Jul 15, 2024

@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:

  • Do you guys see any more views at this point where you would like to see Bottom Tab? (asking because the issue started with Workspaces Page). Because I believe adding a bottom tab on every Page is not something we want? (chats? settings?)

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@JmillsExpensify
Copy link

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.

@melvin-bot melvin-bot bot added the Overdue label Jul 20, 2024
@mountiny
Copy link
Contributor

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

@melvin-bot melvin-bot bot removed the Overdue label Jul 21, 2024
@Kicu
Copy link
Contributor

Kicu commented Jul 22, 2024

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:

  • popular mobile apps have bottom bar on some views but not on all of them, like you would expect...
  • however desktop apps very often don't have bottom bar, even if their mobile versions has it
  • mobile slack shows the bottom tab on different views on ios and android. For example slack chat on iphone will have the bottom bar and on android it will not - I find that interesting but dunno why it's like that;

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 😅

rec-screen.mov

^ 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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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.

@dannymcclain
Copy link
Contributor

@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
Copy link
Contributor

adamgrzybowski commented Sep 23, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

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.

@adamgrzybowski
Copy link
Contributor

Hi! Here is a short update from our side. Recently @WojtekBoman and I had to help with some other navigation issues so progress on persistent bottom tabs wasn't big.

We continued our work on two parts requiring adjustment before testing and improving performance.

  • goBack <- We are halfway finished. It looks like we can finally get this function right without code spaghettification 🤞

  • getAdaptedState <- Also halfway finished

@trjExpensify
Copy link
Contributor

without code spaghettification 🤞

Actually lol'd at that 😆

@adamgrzybowski
Copy link
Contributor

Hi guys! A quick update from us.

getAdaptedSatet
I pushed changes with the new getAdaptedState. Tomorrow I will test it more and look for regressions. There may be some because it handles a lot of cases but it seems pretty stable so far.

goBack
While working on the goBack function, we looked through the archives📜 of the design docs for the first navigation refactor. It described two similar actions: go back and go up.

Currently, the goBack function handles both cases by looking at the fallbackUrl and a few other indicators. This approach may be a bit outdated after so many changes to the nav structure since then. Also, the coupling of these two actions in one function is why it is so difficult to refactor. We are considering splitting it into two separate functions: goBack and goUp as a final API for other contributors.

I wonder if you could take a look at the goUp and goBack documentation and tell us what API you would expect as a developer after reading it. Your feedback would be very valuable 🙇

I found the docs here but not sure if you all have access.

A summary of the differences is described here

performance
As you can see, performance is well... 😄 far from ideal. We identified two main reasons.

  1. We don't have optimization for multiple split navigators in the stack.

  2. Now we render the bottom tab bar per page and it's much more expensive than we initially thought.

We have a solid idea for the first problem and a few loose ones to check for the second problem, but as mentioned before, we want to take care of performance at the very end.

@JmillsExpensify
Copy link

P.S. @marcaaron curious for your thoughts, as you were involved in the original go back and go up implementation.

@mountiny
Copy link
Contributor

mountiny commented Oct 8, 2024

I wonder if you could take a look at the goUp and goBack documentation and tell us what API you would expect as a developer after reading it. Your feedback would be very valuable 🙇

Yep, I think the main difference between Up and Back is considering a deep link to some deeper lever of the stack. Consider you deep link from chat to settings/profile/address/country, and then you click the chevron UP. You go to the address page -> one page up in the stack, even if it was not technically in the browser history. If you go back after deep linking to that page, you just go back to the chat page where you came from.

I guess the confusion is that most of the time, both actions/ buttons do the same. Normally, you navigate through the stack, so going up also means going back in the browser history.

And as such, I am not sure if we need to divide these up to two different API actions for the developer, I kind of feel like the developer should not worry about it much

@adamgrzybowski
Copy link
Contributor

@mountiny We are currently reviewing all the use cases for goBack. You may be right that the only function that the developer actually needs is the one calling the goUp action and there is no need for a separate goBack.

I am wondering that even if that's the case maybe we should rename it to goUp or something like that? It's not really a goBack if we deeplink and it can be confused with the goBack from the browser/android physical button which behaves differently.

@mountiny
Copy link
Contributor

mountiny commented Oct 8, 2024

As long as the documentation is clear, we can name is goUp, but I think it makes more sense in stack. I am sure there are cases where goBack on the other hand makes more sense

@WojtekBoman
Copy link
Contributor

Hey, yesterday I published an update from me and @adamgrzybowski in our PR. To increase the visibility of the post, I am posting a link to it here :)

@dylanexpensify dylanexpensify moved this to Release 3: Fall 2024 (Nov) in [#whatsnext] #expense Oct 18, 2024
@melvin-bot melvin-bot bot added the Overdue label Oct 18, 2024
@mountiny
Copy link
Contributor

This is still in progress

@melvin-bot melvin-bot bot removed the Overdue label Oct 22, 2024
@adamgrzybowski
Copy link
Contributor

The first batch of bugs has arrived 😄 Huge kudos to @lanitochka17 and @Iuliia537 for a great job with testing 🙇 It is just what we needed to push this PR forward. Tomorrow we'll start the bug hunt 🎯 I looked at them briefly and I don't see anything worrying besides the blank page that should be resolved with native stack.

@melvin-bot melvin-bot bot added the Overdue label Oct 31, 2024
@mountiny
Copy link
Contributor

mountiny commented Nov 1, 2024

Getting through the testing feedback

@melvin-bot melvin-bot bot removed the Overdue label Nov 1, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 11, 2024
@mountiny
Copy link
Contributor

mountiny commented Nov 11, 2024

more great progress on the PR

@melvin-bot melvin-bot bot removed the Overdue label Nov 11, 2024
@garrettmknight garrettmknight added the Internal Requires API changes or must be handled by Expensify staff label Nov 19, 2024
@melvin-bot melvin-bot bot added the Overdue label Nov 20, 2024
@mountiny
Copy link
Contributor

progressing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Requires API changes or must be handled by Expensify staff Weekly KSv2
Projects
Status: Bugs and Follow Up Issues
Status: Release 2.5: SuiteWorld (Sept 9th)
Development

No branches or pull requests

9 participants