deps: Upgrade @react-navigation/{bottom,material-top}-tabs to 6.x #5857
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This version should correspond better with RN 68, which is what we're on.
When we upgrade @react-navigation/material-top-tabs soon, it will no longer support putting tabs inside a ScrollView: react-navigation/react-navigation#11067 (comment) So, wrap the content of each tab, instead of wrapping the whole tab navigator, taking care to copy the ScrollView's effective configuration.
This unfortunately causes a peer-dep warning: warning " > @react-navigation/[email protected]" has incorrect peer dependency "@react-navigation/native@^6.0.0". But no issues have yet appeared in manual testing, and that's consistent with the note on the React Navigation upgrade guide that says, "To make upgrading easier, it is possible to mix packages from the `6.x.x` and `5.x.x` version ranges.": https://reactnavigation.org/docs/upgrading-from-5.x#note-on-mixing-react-navigation-5-and-react-navigation-6-packages Changelog: https://github.com/react-navigation/react-navigation/blob/main/packages/material-top-tabs/CHANGELOG.md Done by reading and following the relevant parts of the React Nav upgrade guide, including general information at the top and also the section specific to Material Top Tabs: https://reactnavigation.org/docs/upgrading-from-5.x/#material-top-tab-navigator Done now because it lets us get rid of react-native-reanimated, as foreshadowed in our React Nav 6 upgrade issue: zulip#4936 (comment) That's helpful because the old version of react-native-reanimated that we're on uses a certain iOS API that Apple identifies as privacy-sensitive, triggering a "Privacy Manifest" requirement: software-mansion/react-native-reanimated#5819 For zulip#5847, we're working on reducing and handling such requirements. That API usage is removed in v2.9.0-rc.0 of Reanimated (see just-linked PR), but I didn't pursue upgrading it because that path seems to require abandoning remote JS debugging, at least according to a note from 2022. For details on that, search for "react-native-reanimated" here: zulip#5441 Related: zulip#5847 Fixes-partly: zulip#4936
This resolves a helpful performance warning logged to the console
while the app runs:
('upside_down' is the name of the upside-down smiley face emoji;
it's one of the tabs on the reactions screen for a particular
message.)
Looks like you're passing an inline function for 'component' prop
for the screen 'upside_down' (e.g.
component={() => <SomeComponent />}).
Passing an inline function will cause the component state to be
lost on re-render and cause perf issues since it's re-created
every render. You can pass the function as children to 'Screen'
instead to achieve the desired behaviour.
…m tabs This was quietly dropped in v5, it seems: https://reactnavigation.org/docs/4.x/bottom-tab-navigator/ https://reactnavigation.org/docs/5.x/bottom-tab-navigator/ The icons are still shown in manual testing, thankfully. While we're at it, make BottomTabBarOptions exact in types/ using a TsFlower patch.
This unfortunately adds two more peer-dep warnings to the one that appeared when we upgraded @react-navigation/material-top-tabs, earlier in this series. Here are all three together: warning " > @react-navigation/[email protected]" has incorrect peer dependency "@react-navigation/native@^6.0.0". warning "@react-navigation/bottom-tabs > @react-navigation/[email protected]" has incorrect peer dependency "@react-navigation/native@^6.0.0". warning " > @react-navigation/[email protected]" has incorrect peer dependency "@react-navigation/native@^6.0.0". But again, no issues have appeared in manual testing; see note in that earlier commit. Changelog: https://github.com/react-navigation/react-navigation/blob/main/packages/bottom-tabs/CHANGELOG.md For why it's OK to abandon our own custom version of this dependency, see the commit where we started using it: c0b17bd > So this fix will become unnecessary when we're on React > Navigation 6 Done by reading and following the relevant parts of the React Nav upgrade guide, including general information at the top and also the section specific to Bottom Tabs: https://reactnavigation.org/docs/upgrading-from-5.x#bottom-tab-navigator Done with the hope that it wouldn't take too much effort to get to the finish line of the React Nav v6 upgrade, zulip#4936. But when I tried upgrading @react-navigation/stack and @react-navigation/native, I had a lot of trouble working out the right Flow types and TsFlower patches, and this is a legacy codebase. But this part was doable. Fixes-partly: zulip#4936
|
I took screenshots on my iPhone yesterday, and I forgot which ones were "before" and which ones were "after" shots. I'll include both sets anyway and call them "A" and "B". If we spot important differences in how the UI looks between A and B, I can find out which is "before" and "after" by building the app again. But I think I can save quite a bit of time by just posting the ones I have now. Bottom tabs:
Reactions:
Android screenshots coming next. |
Android screenshots:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The @react-navigation/material-top-tabs upgrade is done to get us off of react-native-reanimated, which should help us with
Then the @react-navigation/bottom-tabs upgrade was done because I was optimistic about swiftly finishing
. But I didn't manage to finish that off; it turned out to be more work than I have time for right now (in particular, upgrading @react-navigation/native and @react-navigation/stack). But the bottom-tabs upgrade, included here, should be ready to go.
Screenshots coming, to demonstrate that the appearance doesn't change except in one small way where we were forced to make an adjustment:
Related: #5847
Fixes-partly: #4936