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-05-29] [Wave Collect] [Ideal nav] Simplify the navigator structure #40681

Closed
mountiny opened this issue Apr 22, 2024 · 16 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2

Comments

@mountiny
Copy link
Contributor

mountiny commented Apr 22, 2024

Follow up to the ideal navigation project, we have one task to simplify the structure now.

Problem

Currently, central pane screens in FullScreenNavigator are wrapped with the central pane navigator. A similar pattern can be seen in LHP where single screens are wrapped with navigators.
This may be problematic for two reasons:

  • Getting information about the actual screen requires more code. It has become visible especially after we migrated to typescript. Instead of just getting the route name we have to extract the route from the state or params of the parent navigator.
  • More nested navigators means worse performance.

Solution

In the early implementation of navigation, we used the custom component ThreePaneView. That was the main reason to encapsulate screens with central pane navigators. Now we use StackView for both narrow and wide layouts and the wide layout is achieved using styles.

This allows us to flatten the navigation structure. Now screens in FullScreenNavigator and LHP will be mounted directly
This way developing and maintaining navigation should be easier and we should get a slight performance boost.

cc @adamgrzybowski @WojtekBoman

@mountiny mountiny added Daily KSv2 NewFeature Something to build that is a new item. labels Apr 22, 2024
@mountiny mountiny self-assigned this Apr 22, 2024
Copy link

melvin-bot bot commented Apr 22, 2024

Triggered auto assignment to @stephanieelliott (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.

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Apr 22, 2024
@WojtekBoman
Copy link
Contributor

Hi! I'm working on that :)

@stephanieelliott stephanieelliott added NewFeature Something to build that is a new item. and removed NewFeature Something to build that is a new item. labels Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

Triggered auto assignment to @strepanier03 (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.

@stephanieelliott
Copy link
Contributor

Reapplying the New Feature label to get another BZ member on this while I am OOO til May 2. Thanks @strepanier03, I'll grab this back from you when I return!

@melvin-bot melvin-bot bot added the Overdue label May 1, 2024
@stephanieelliott
Copy link
Contributor

I'm back from OOO, thanks for watching this while I was out @strepanier03!

Hey @WojtekBoman, any update on when this PR will be taken out of the Draft state?

@melvin-bot melvin-bot bot removed the Overdue label May 2, 2024
@stephanieelliott
Copy link
Contributor

Hey @WojtekBoman, any update on when this PR will be taken out of the Draft state?

Bump on this @WojtekBoman, how is the PR coming?

@WojtekBoman
Copy link
Contributor

Hi @stephanieelliott,

I'm currently working on the new search tab as it has higher priority. I think I'll have some time to release a PR this/next week.

@stephanieelliott
Copy link
Contributor

Makes sense, thanks for the update @WojtekBoman!

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Weekly KSv2 labels May 10, 2024
@stephanieelliott
Copy link
Contributor

Hey @WojtekBoman, any update on this?

@mountiny
Copy link
Contributor Author

This was merged and deployed to staging, awaiting production deploy

@stephanieelliott
Copy link
Contributor

This was deployed to prod on May 22! Will be ready for payment on May 29

@stephanieelliott stephanieelliott added the Awaiting Payment Auto-added when associated PR is deployed to production label May 29, 2024
@stephanieelliott stephanieelliott changed the title [Wave Collect] [Ideal nav] Simplify the navigator structure [HOLD for payment 2024-05-29] [Wave Collect] [Ideal nav] Simplify the navigator structure May 29, 2024
@mountiny
Copy link
Contributor Author

@ahmedGaber93 is the only person required payment of $250 on this issue

No regression testing required

Copy link

melvin-bot bot commented May 29, 2024

Payment Summary

Upwork Job

BugZero Checklist (@stephanieelliott)

  • I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
  • I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
  • I have paid out the Upwork contracts or cancelled the ones that are incorrect
  • I have verified the payment summary above is correct

@stephanieelliott
Copy link
Contributor

I had to manually create a job on Upwork for this

@ahmedGaber93 please let me know when you've accepted the offer so I can issue payment!

@ahmedGaber93
Copy link
Contributor

@stephanieelliott Offer accepted. Thanks!

@stephanieelliott
Copy link
Contributor

Thanks @ahmedGaber93, all paid!

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 NewFeature Something to build that is a new item. Reviewing Has a PR in review Weekly KSv2
Projects
Archived in project
Development

No branches or pull requests

5 participants