Implement React Navigation #1559
Conversation
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
Do a lot Fix up routing issues on web by using Linking.getInitialUrl() Make sure root route displays correctly fix favicons fix background issue for native modals Fix report not loading on init remove isSidebarShown code move header gaps fix weird window width issue
marcaaron
force-pushed
the
marcaaron-reactnav
branch
from
b6ad55d to
3d9e274
Compare
|
@Dal-Papa sorry mean to make this a draft 🙃 |
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
4 similar comments
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work? |
|
@shawnborton lmk if you are up for taking this for a spin and bringing back any notes before we go into a full review. I tried to keep everything largely the same, but might have missed some small details here and there. |
|
Just took this for a quick spin on iOS and it feels pretty smooth to me! I think this might currently exist, but I do think it's weird that on mobile devices, we use an active state for the chat row when in reality you aren't really viewing an active chat: Only other thing I ran into was that I am unable to create new group chats - not sure if that is a result of this PR or not. Otherwise this is feeling awesome, such a great improvement! |
Oh yeah, that's a great catch. Will look into this. |
|
Updated |
|
Ok, ended up responding to most of the comments! Removing |
|
Thanks @Julesssss! I've gone ahead and created some follow up issues for the things we've discussed, but didn't want to tackle yet. They can be found in the project column here. Let me know if there's anything I missed that needs an issue. |
@roryabraham managed to remove all of those cases and re-organize a bit definitely looks tidier |
There was a problem hiding this comment.
All good 👍
Noticed a couple of minor things during a final run-through, but no need to view these as blockers:
- [Web]: the modal-close animation seems to have gone, but the modal-open animation still works
- [Android] (Physical device): when you open the settings page the first back button press is ignored (as if an extra invisible page was added to the page stack), but the second and third back button press work as expected
|
Created issues for those as well, thanks. |
There was a problem hiding this comment.
This is great, amazing job @marcaaron 🎉 Tested on desktop + android sim and works well.
Couple things to note:
[Android] (Physical device): when you open the settings page the first back button press is ignored (as if an extra invisible page was added to the page stack), but the second and third back button press work as expected
I didn't experience this same thing on the android sim, all buttons worked immediately on the first press.
Secondly, I was getting some delay with the chat switcher on android sim only (so I'm not sure this is a real issue but figured I'd call it out just in case) where the old chat was momentarily displayed before the new chat
sim-chatswitcher.mov
marcaaron
dismissed stale reviews from AndrewGable, NikkiWines, and roryabraham
via
2b5e557
|
So close, but got just one more conflict 😝 updated |
|
|
||
| const IOURequestModalStackNavigator = () => ( | ||
| <IOURequestModalStack.Navigator | ||
| path="/iou/request" |
There was a problem hiding this comment.
Would it be OK to change these to reference ROUTES.js?
| import themeColors from '../../styles/themes/default'; | ||
|
|
||
| const propTypes = { | ||
| authenticated: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
These are missing inline docs
| throw new Error("Couldn't find a navigation object. Is your component inside a screen in a navigator?"); | ||
| } | ||
|
|
||
| const state = linkingConfig?.getStateFromPath |
There was a problem hiding this comment.
Are we officially adopting the ?. pattern now?
| root.reset(state); | ||
| } | ||
| } else { | ||
| throw new Error('Failed to parse the path to a navigation state.'); |
There was a problem hiding this comment.
Our standard style is to fail quickly and early, so this should be moved up to an if (!state) piece of logic
| } | ||
| } | ||
|
|
||
| Onyx.merge(ONYXKEYS.CURRENT_URL, path); |
There was a problem hiding this comment.
Move all Onyx calls into an action (discussed on Slack briefly)
| redirectTo: PropTypes.string, | ||
| // Session info for the currently logged in user. | ||
| session: PropTypes.shape({ | ||
| authToken: PropTypes.string, |
There was a problem hiding this comment.
A lot of sub-props in this PR are missing inline comments
| getReportRoute: reportID => `/r/${reportID}`, | ||
| ROOT: '/', | ||
| SEARCH: '/search', | ||
| SET_PASSWORD: '/setpassword/:validateCode', |
There was a problem hiding this comment.
It looks like some of these route definitions are no longer used. Can these be cleaned up?
| const propTypes = { | ||
| // Internal react-navigation stuff used to determine which view we should display | ||
| state: PropTypes.shape({ | ||
| index: PropTypes.number, |
There was a problem hiding this comment.
More inline comments missing
|
|
||
|
|
||
| class ResponsiveView extends React.Component { | ||
| getCurrentViewDescriptor() { |
| </Text> | ||
| </Pressable> | ||
| <ScreenWrapper> | ||
| {() => ( |
There was a problem hiding this comment.
I was seeing all of these, and wondering if there is a way to clean these up a little. I might not have all the context behind this, but I looked through the docs for the safe area context: https://github.com/th3rdwave/react-native-safe-area-context
It appears to me that the only reason the {() => ()} pattern is used is because ScreenWrapper is using SafeAreaInsetsContext.Consumer to get access to the insets. The insets are only used in a single component (sidebar).
I'm curious if all the other views could be much more simple that don't need access to the insets. Maybe something like this:
// Normal screenwrapper defaults to not providing insets
<ScreenWrapper>
<View />
</ScreenWrapper>
Then for the one that needs insets:
<ScreenWrapperWithInsets>
{(insets) => (...)}
</ScreenWrapperWithInsets>
cc @AndrewGable @Julesssss
Details
react-navigationand removesreact-routerfromExpensify.cash.It's HOLDing on this issue since we are still not redirecting unknown paths back toDONE!index.htmland this is a requirement for deep linking to work (although routing in general will work fine)There are a few known issues that I'm still working on resolving:
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/154512
Tests
Will be adding some more tests, but here's what I have so far...
mWeb
/r/<reportID>in a new mobile web window./new/chat/new/group/settings/searchWeb/Desktop wide screen
/r/<reportID>in a new mobile web window./new/chat/new/group/settings/searchiOS / Android
Extra steps:
/settings/profile,/settings/password, etcTested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android