-
Notifications
You must be signed in to change notification settings - Fork 48
feat: add Tab api support on newer OS #364
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.
Thanks for working on this!
I've found some issues with current implementation that we need to sort out.
.measureView { size in | ||
onLayout(size) | ||
} |
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.
Can you check if measureView returns the same values? Before we applied it to the ForEach, now it's applied to the TabView. I'm afraid this can cause issues and return different values
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.
hi, when testing i haven't had any problems with measureView applied to TabView, and found that it would always return one value/the same value as if measureView was applied to Tab child. also i tried moving it up to the ForEach, but it gave this error Referencing instance method 'measureView(onLayout:)' on 'ForEach' requires that 'some TabContent<String>' conform to 'View'
which i did not know how to resolve. Let me know if you think we should rework it and apply to the ForEach anyway, otherwise i'd leave it like this since it seems like nothing changes
labeled: props.labeled | ||
) | ||
} | ||
//.badge(tabData.badge) |
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.
Why it's commented out?
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.
as i wrote in the pr description, i commented it out because otherwise on iOS, even if tabData.badge is an empty string, a badge is created on the tab bar (see screenshot attached below). this behaviour can be seen on both ios 18 and 26, however it doesn't happen on ipad or other platforms. i tried using an if statement (like copilot suggested) but it broke the ForEach with Cannot convert value of type 'Range<Array<PlatformView>.Index>' (aka 'Range<Int>') to expected argument type 'Binding<C>'
. i also tried using view modifiers, moving that part to a separate functio, using tabData.badge.isEmpty ? nil : tabData.badge
(which does not work because .badge does not accept nil as value) and tabData.badge.isEmpty ? 0 : tabData.badge
(which does not work because a variable can't be of type Int or String), but nothing worked and it seems like the only way to remove it is either not call the function or call it with 0 (which can't be done since you either call it with a number or a string). let me know your thoughts on this and how we could resolve it, because honestly i have no idea
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 think that's because it's supposed to be Text?
not String?
https://developer.apple.com/documentation/swiftui/view/badge(_:)-6k2u9
This compiled for me
.badge(tabData.badge.isEmpty ? nil : Text(tabData.badge))
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.
thanks for pointing it out. when developing i only saw and looked into the one below, which didn't specify text nor string
https://developer.apple.com/documentation/swiftui/tabcontent/badge(_:)
currently i can't test your implementation, however if you say it works with Text then this might be the solution. can you confirm the problem is gone on both ios 18.5 and ios 26 simulators?
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.
packages/react-native-bottom-tabs/ios/TabView/LegacyTabView.swift
Outdated
Show resolved
Hide resolved
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.
To make TabRole
work in the PR
|
||
if !tabData.hidden || isFocused { | ||
let icon = props.icons[index] | ||
let role: TabRole? = nil |
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.
We can look for a String
of search
(To make this backwards-compatible <18.0) and then convert that into a TabRole.search
let role: TabRole? = nil | |
var role: TabRole? { | |
if tabData.tabRole == "search" { | |
return TabRole.search | |
} | |
return nil | |
} |
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.
Of course we'd have to add the tabRole
prop to TabViewProvider
as well
diff --git a/packages/react-native-bottom-tabs/ios/TabViewProvider.swift b/packages/react-native-bottom-tabs/ios/TabViewProvider.swift
index 135c9a9..63669b5 100644
--- a/packages/react-native-bottom-tabs/ios/TabViewProvider.swift
+++ b/packages/react-native-bottom-tabs/ios/TabViewProvider.swift
@@ -13,7 +13,8 @@ public final class TabInfo: NSObject {
public let activeTintColor: PlatformColor?
public let hidden: Bool
public let testID: String?
-
+ public let tabRole: String?
+
public init(
key: String,
title: String,
@@ -21,7 +22,8 @@ public final class TabInfo: NSObject {
sfSymbol: String,
activeTintColor: PlatformColor?,
hidden: Bool,
- testID: String?
+ testID: String?,
+ tabRole: String?
) {
self.key = key
self.title = title
@@ -30,6 +32,7 @@ public final class TabInfo: NSObject {
self.activeTintColor = activeTintColor
self.hidden = hidden
self.testID = testID
+ self.tabRole = tabRole
super.init()
}
}
@@ -267,7 +270,8 @@ public final class TabInfo: NSObject {
sfSymbol: itemDict["sfSymbol"] as? String ?? "",
activeTintColor: RCTConvert.uiColor(itemDict["activeTintColor"] as? NSNumber),
hidden: itemDict["hidden"] as? Bool ?? false,
- testID: itemDict["testID"] as? String ?? ""
+ testID: itemDict["testID"] as? String ?? "",
+ tabRole: itemDict["tabRole"] as? String ?? ""
)
)
}
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.
makes sense to me, however i don't know if setting tabRole to a String? would be the right move, maybe using an Enum would make it more typesafe and future proof. Also, similarly to tabBarCustomization, it's probably better to add it in a follow up pr after this one is merged.
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.
Yeah String?
isn't super ideal but the TabRole
enum is only available in iOS18+ which would mean splitting into NewTabViewProvider
and LegacyTabViewProvider
as well as the split you already have.
There is currently only one role: .search but I think we could do TabRole(rawValue: tabData.tabRole)
though which would still future proof it in case of future roles.
For sure can be done in a followup PR, it would obviously build upon yours and I couldn't figure out how to get GitHub to stack PRs
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.
you're right, that's probably the easiest way atm, and i guess we could implement the enum on the TS types (type TabRole = 'search';
) so typescript users will have a typesafe way of adding a tab role. As for pr stacking i don't know either, we might have to wait for this one to be merged first
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 would remove passing role in this PR completely and create a separate PR implementing it. For PR stacking you can create a new branch off this PR and create a new PR pointing to this branch.
Making it a string sounds good to me, we can use TypeScript to enforce users to pass search
and if in the future Apple adds more roles we will be ready.
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.
Pull Request Overview
This PR introduces support for Apple’s new SwiftUI Tab API on iOS/iPadOS/tvOS 18+, macOS 15+, and visionOS 2+, while maintaining the existing implementation for earlier OS versions.
- Extracted
NewTabView
andLegacyTabView
and toggled between them inTabViewImpl
based on platform availability. - Moved tab appearance logic into
TabAppearModifier
and unified view signatures under a newAnyTabView
protocol. - Simplified
TabViewImpl
by replacing inlinerenderTabItem
logic with atabContent
computed property.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
packages/react-native-bottom-tabs/ios/TabViewImpl.swift | Replaced inline TabView with conditional NewTabView /LegacyTabView and removed renderTabItem . |
packages/react-native-bottom-tabs/ios/TabView/NewTabView.swift | New SwiftUI 18+ implementation using Tab(value:role:) and custom appearances. |
packages/react-native-bottom-tabs/ios/TabView/LegacyTabView.swift | Legacy implementation extracted from original code, preserving pre-18 API. |
packages/react-native-bottom-tabs/ios/TabView/AnyTabView.swift | Introduced protocol to unify new and legacy tab view interfaces. |
packages/react-native-bottom-tabs/ios/TabAppearModifier.swift | Added modifier to centralize onAppear logic for tab updates. |
Comments suppressed due to low confidence (1)
packages/react-native-bottom-tabs/ios/TabView/NewTabView.swift:13
- There are currently no tests covering
NewTabView
’s behavior with the new SwiftUI Tab API. Consider adding unit or snapshot tests for selection, badge display, and role assignment on supported OS versions.
TabView(selection: $props.selectedPage) {
|
||
if !tabData.hidden || isFocused { | ||
let icon = props.icons[index] | ||
let role: TabRole? = nil |
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.
The TabRole is hardcoded to nil, which prevents assigning roles like .search from your tabData. Consider mapping the role from tabData (e.g., let role: TabRole? = tabData.role
) to restore role support.
let role: TabRole? = nil | |
let role: TabRole? = tabData.role |
Copilot uses AI. Check for mistakes.
labeled: props.labeled | ||
) | ||
.accessibilityIdentifier(tabData.testID ?? "") | ||
.tag(tabData.key) |
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.
The .tag
modifier is placed inside the .tabItem
closure, so it may not be applied correctly to the TabView item. Move .tag(tabData.key)
outside of the .tabItem { … }
builder to ensure the TabView recognizes the tag.
.tag(tabData.key) |
Copilot uses AI. Check for mistakes.
@ViewBuilder | ||
var tabContent: some View { | ||
if #available(iOS 18, macOS 15, visionOS 2, tvOS 18, *) { | ||
NewTabView( |
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.
The onLongPress
closure from TabViewImpl
isn’t forwarded into NewTabView
, so long-press events will no longer trigger. Pass onLongPress
into both NewTabView
and LegacyTabView
if you need to support it.
Copilot uses AI. Check for mistakes.
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.
We need to fix this
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.
We are getting there! Thanks for applying the fixes.
Sorry for taking so long to review but I got sick last week 🥲
I'm going to merge this PR soon: #378
Can you rebase and implement the badge behavior properly? Afterwards we should be good to merge, also can you run swiftlint --fix ./packages
in the root of the repo?
|
||
if !tabData.hidden || isFocused { | ||
let icon = props.icons[index] | ||
let role: TabRole? = nil |
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 would remove passing role in this PR completely and create a separate PR implementing it. For PR stacking you can create a new branch off this PR and create a new PR pointing to this branch.
Making it a string sounds good to me, we can use TypeScript to enforce users to pass search
and if in the future Apple adds more roles we will be ready.
no problem, take your time to recover. |
PR Description
This pr adds support for the new swift Tab api on ios/ipadOS/tvOS 18+, macOS 15+ and visionOS 2+. This let us implement many new features (such as search role for the tab item, tabbar customization, swipe actions and context menus), while also opening up an opportunity to implement #296 in the future.
In order to implement it i had to create a separate file for each implementation because only by doing so and using
@available(iOS 18, macOS 15, visionOS 2, tvOS 18, *)
onNewTabView
i could then add@AppStorage("sidebarCustomizations")
. I tried to maintain every feature on each implementation and moved theTabViewImpl#renderTabItem
toLegacyTabView#renderTabItem
. i was hoping to add a similar function to the NewTabView struct but in that case the Tab api gave an error about not conforming to View (strange since the same code works directly in the ForEach).As of now i have disabled customization for each Tab (using
.customizationBehavior(.disabled, for: .sidebar, .tabBar)
), as we can then discuss in a followup pr/issue the js implementation, set Tab role to nil (same reason), disabled badges on the new tab (commented line) because even iftabData.badge
is an empty string, an empty red dot is created, and i could not figure out how to circumvent this. Finally, i found a strange behaviour with theNative Bottom Tabs with Custom Tab Bar
example, where for the new tab implementation, the native tab is still visible, but even there i could not figure out why.Let me know your thoughts and if you have any suggestion on how to resolve these issues
How to test?
Open the app and test the examples
Screenshots
N/A, the look is the same, only a new edit button has appeared in the ipad/macos sidebar