-
Notifications
You must be signed in to change notification settings - Fork 3k
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-07-10] [$250] Simplify the RootNavigator structure #42507
Comments
Triggered auto assignment to @JmillsExpensify ( |
Job added to Upwork: https://www.upwork.com/jobs/~01e857cb7b9e57642a |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @hoangzinh ( |
@WojtekBoman, @Kicu or I will take care of it |
Hi! I'm working on it :) |
📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
📣 @hoangzinh 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app! |
@JmillsExpensify, @hoangzinh, @mountiny, @WojtekBoman, @adamgrzybowski Eep! 4 days overdue now. Issues have feelings too... |
The PR is still in draft |
The PR is ready for review, there is one unit tests pipeline that is failing, I'm currently investigating it, but besides that everything is ready |
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. |
This issue has not been updated in over 15 days. @JmillsExpensify, @hoangzinh, @mountiny, @WojtekBoman, @adamgrzybowski eroding to Monthly issue. P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do! |
This will be ready to be paid soon |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 9.0.3-7 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-07-10. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
@hoangzinh do you mind filling out the BZ checklist? |
@JmillsExpensify this was a new feature/ follow up so no other PR to flag this on. $250 to @hoangzinh for their review and testing. |
Also no regression test required |
@JmillsExpensify, @hoangzinh, @mountiny, @WojtekBoman, @adamgrzybowski Uh oh! This issue is overdue by 2 days. Don't forget to update your issues! |
@hoangzinh the old offer expired so I just sent you a new one. |
Accepted. Thanks @JmillsExpensify |
Awesome thanks! All paid out. |
Follow up to the ideal navigation project, we have one task to simplify the structure now.
Problem
The problem is the same as described in this issue but for the
RootNavigator
.Currently, central pane screens in
RootNavigator
are wrapped with the central pane navigator. This may be problematic for two reasons: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 from
CentralPaneNavigator
will be mounted directly in theRootNavigator
This way developing and maintaining navigation should be easier and we should get a slight performance boost.
cc @adamgrzybowski @WojtekBoman
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @JmillsExpensifyThe text was updated successfully, but these errors were encountered: