search

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.1k stars 2.6k forks source link

LOW: Investigate using native-stack on native platforms instead of stack #29948

Open mountiny opened 8 months ago

mountiny commented 8 months ago

Coming from here

When we have refactored the App navigation, we have chose to use the normal react-navigation stack as that gave us more control over animation/ transitions of pages on web. This was to also not complicate the MVP for the project with two types of stack.

The problem is that the normal stack does not lead to native feel of app page transitions in native device which makes the feel of the app worse as the page transitions are ever so slightly off compared to the other native apps you might be using on daily basis.

Its important to address this discrepancy and ideally introduce the native-stack for native iOS and Android platforms, as chat/ P2P users have high standards for native app quality.

cc @hannojg @adamgrzybowski

Issue OwnerCurrent Issue Owner: @kirillzyusko
hannojg commented 8 months ago

Yes, will help investigating this or next week 🤝!

hannojg commented 8 months ago

PR is up:

hannojg commented 8 months ago

We got two blockers at the minute:

WoLewicki commented 8 months ago

@hannojg as for the second dot, the ios like do you think that this animation is what people want on Android to have the native feel? Maybe we could explore the other animations that are strictly taken from the Android XMLs?

Also, could you elaborate how does native-stack improve the performance of the app exactly? One thing I can think of is native swipe to go back on iOS which is controlled by the native code, but you still need to mount the previous screen on the native side when it happens, did you check how it performs when going back to reports with many many messages?

hannojg commented 8 months ago

ios like do you think that this animation is what people want on Android to have the native feel

We have to double check the animation. Its called iOS like but I think it actually feels like the android default slide transition

Also, could you elaborate how does native-stack improve the performance of the app exactly

This is more about perceived performance and UX feeling. Right now you can tell by using the expensify app, that the transitions aren't the native transitions. So we want to have the native transitions so expensify feels like a native app.

you still need to mount the previous screen on the native side when it happens, did you check how it performs when going back to reports with many many messages?

Do I understand it correctly that you are suggesting that switching to native-stack could be a performance issue when doing the swipe back gesture?

WoLewicki commented 8 months ago

Do I understand it correctly that you are suggesting that switching to native-stack could be a performance issue when doing the swipe back gesture?

We have the same abstraction in the stack navigator, where we control the mounting/unmounting the views by using activityState prop. Native-stack should not have worse performance, I'm just curious if it is fast enough no to be laggy in high traffic accounts 😅

mountiny commented 8 months ago

we should test it out with high traffic accounts, the E2E tests once are built in like 2 weeks should also tell us more how it performs there

melvin-bot[bot] commented 7 months ago

This issue has not been updated in over 15 days. @hannojg, @mountiny 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!

mountiny commented 7 months ago

Keeping this monthly, its not high priority and still waiting on some issues to get resolved.

melvin-bot[bot] commented 5 months ago

@hannojg, @mountiny, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

mountiny commented 5 months ago

@hannojg How is this looking now with latesst ios, have they fixed anything?

hannojg commented 5 months ago

Hey, I am handing this over to our Chief Keyboard Officer @kirillzyusko who is way more familiar with those keyboard issues. Once he is done with fixing some other keyboard handling issues he'll take a look!

mountiny commented 5 months ago

@kidroca Can you comment on this one?

kirillzyusko commented 5 months ago

I'm ready to pick up this task, so you can assign it on me 👀

mountiny commented 5 months ago

Sorry Kidroca, that was a typo!

@kirillzyusko Lets update the PR to see where we are at now

Julesssss commented 4 months ago

Assigning @abdulrahuman5196 based on this comment

melvin-bot[bot] commented 4 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

melvin-bot[bot] commented 4 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

melvin-bot[bot] commented 4 months ago

⚠️ Looks like this issue was linked to a Deploy Blocker here

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.

kirillzyusko commented 4 months ago

We reverted the PR with native-stack because it had too many issues - https://github.com/Expensify/App/pull/37305

I'll create a new PR where I'll resolve all issues 👍

mountiny commented 4 months ago

Thanks!

melvin-bot[bot] commented 4 months ago

Triggered auto assignment to @anmurali (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

melvin-bot[bot] commented 4 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.45-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-03-07. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 4 months ago

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:

abdulrahuman5196 commented 3 months ago

Not awaiting payment. Still in PR phase since the PR got reverted.

melvin-bot[bot] commented 3 months ago

Payment Summary

[Upwork Job]()

BugZero Checklist (@anmurali)

mountiny commented 3 months ago

@kirillzyusko whats your ETA for this one?

kirillzyusko commented 3 months ago

@mountiny I think early next week (Monday, Tuesday) my PR will be ready for review and testing. I fixed all the issues that were reported for previous PR, but I tested apps side-by-side (old stack navigator and native-stack navigator) and discovered more issues which I'm fixing right now - but the progress is moving pretty good, so I think I'll be able to make something in a good shape next Monday/Tuesday (but we'll see how it goes).

mountiny commented 3 months ago

Thank you! have a good weekend!

melvin-bot[bot] commented 2 months ago

This issue has not been updated in over 15 days. @anmurali, @kirillzyusko, @mountiny, @abdulrahuman5196 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!

mountiny commented 2 months ago

We should be merging the Fabric branch this week

quinthar commented 1 month ago

Should this be closed? If not, what is the next step, who is doing it, and when?

mountiny commented 1 month ago

The PR is actively being worked on https://github.com/Expensify/App/pull/37891, we are addressing some changes required upstream

melvin-bot[bot] commented 4 weeks ago

@anmurali, @kirillzyusko, @mountiny, @abdulrahuman5196, this Monthly task hasn't been acted upon in 6 weeks; closing.

If you disagree, feel encouraged to reopen it -- but pick your least important issue to close instead.

kirillzyusko commented 4 weeks ago

@mountiny should we re-open this?