-
-
Notifications
You must be signed in to change notification settings - Fork 588
fix(iOS, Tabs): update tab bar item only when necessary #3290
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the direction on this PR.
I've just got remarks regarding naming. Let's update that & proceed.
#if !RCT_NEW_ARCH_ENABLED | ||
BOOL _tabItemNeedsAppearanceUpdate; | ||
BOOL _tabScreenOrientationNeedsUpdate; | ||
BOOL _tabBarItemNeedsBaseChange; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When reading the code for the first time the base
part made no sense to me. What we want to signal here is that the tab bar item needs to be recreated & requires allocating completely new instance. What about naming it _tabBarItemNeedsRecreation
then? What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also rename the changeTabBarItem
appropriately. createTabBarItem
?
} else if (screenView.systemItem != RNSBottomTabsScreenSystemItemNone) { | ||
// Restore default system item icon | ||
UITabBarSystemItem systemItem = | ||
rnscreens::conversion::RNSBottomTabsScreenSystemItemToUITabBarSystemItem(screenView.systemItem); | ||
tabBarItem.image = [[UITabBarItem alloc] initWithTabBarSystemItem:systemItem tag:0].image; | ||
} | ||
|
||
if (screenView.selectedIconSfSymbolName != nil) { | ||
tabBarItem.selectedImage = [UIImage systemImageNamed:screenView.selectedIconSfSymbolName]; | ||
} else if (screenView.systemItem != RNSBottomTabsScreenSystemItemNone) { | ||
// Restore default system item icon | ||
UITabBarSystemItem systemItem = | ||
rnscreens::conversion::RNSBottomTabsScreenSystemItemToUITabBarSystemItem(screenView.systemItem); | ||
tabBarItem.selectedImage = [[UITabBarItem alloc] initWithTabBarSystemItem:systemItem tag:0].selectedImage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this trick.
Description
Limits changing tab bar item instances (PR introducing this) to only necessary cases (changing
systemItem
). Unnecessary updates caused a bug when changing e.g. onlybadgeValue
.before.mov
after.mov
Fixes #3273.
Changes
UITabBarItem
instance and updating tab bar item related propsUITabBarItem
instance only onsystemItem
changeUITabBarSystemItem
instance instead of changing the entireUITabBarItem
Test code and steps to reproduce
Run
TestBottomTabs
. ChangebadgeValue
on selected tab. You can also check test cases from #3164.Checklist