refactor(Tabs, Android): Split TabsHost into TabsHost and TabsContainer#3766
Merged
refactor(Tabs, Android): Split TabsHost into TabsHost and TabsContainer#3766
TabsHost into TabsHost and TabsContainer#3766Conversation
Collaborator
|
Manual test were perform using Android emulator and FabricExample App. Functionalities covered in tests:
Not covered in tests:
|
kkafar
force-pushed
the
@kkafar/tabs-refactor-4
branch
from
c0a9bb2 to
e015bb5
Compare
I've moved all functionalities related to: 1. safe area, 2. color scheme, 3. tab bar hidden, 4. container update, 5. a11y, 6. appearance (and possibly more) to `TasbContainer`. `TabsHost` is now only a source of configuration, layout and `TabsScreens`. Notably, I've left the logic related to `layoutCallback` and layout refresh inside `TabsHost`. I'm not sure where this logic should live. In the future we might to move it from there. One thing to note here is that `TabsContainer` relies on it - now via call to `this.requestLayout`, which calls the overridden function on the `TabsHost` (view parent). **For the transition period**, I also pass to `TabsContainer` a reference to `TabsHost`. This obviously creates circular dependency, which is never resolved, but as I've learned recently from @kligarski it shouldn't be a problem for the GC. This will be changed later, when I refactor communication patterns. Please don't mind commented out code / leftovers in this commit. I intend to clean them up later, in follow up commits. Regarding functionality - ideally everything should work just as before. I don't want to perform full manual scan right now, as more "big" changes are planned before we land this one. I've only asserted that basic functionality works as intended on `TestBottomTabs`.
It does not overryde anything yet
kkafar
force-pushed
the
@kkafar/tabs-refactor-5
branch
from
354d8ee to
ac90649
Compare
kkafar
commented
Mar 24, 2026
|
|
||
| @SuppressLint("ViewConstructor") // Created only by us. Should never be restored. | ||
| internal class TabsContainer(private val context: Context, | ||
| internal val tabsHost: TabsHost // TODO: TEMPORARY, REMOVE THIS DEPENDENCY |
Comment on lines
+72
to
+73
| // TODO: AFTER REFACTOR COMPLETES, THIS SHOULD BE PRIVATE | ||
| internal val tabsModel: MutableList<TabsScreenFragment> = arrayListOf() |
Member
Author
There was a problem hiding this comment.
And this will be made private in follow-up: #3776
Comment on lines
+156
to
+159
| // containerUpdateCoordinator.let { | ||
| // it.invalidateAll() | ||
| // it.runContainerUpdate() | ||
| // } |
| } | ||
|
|
||
| post { | ||
| // refreshLayout() |
Comment on lines
+331
to
+334
| // containerUpdateCoordinator.let { | ||
| // it.invalidateNavigationMenu() | ||
| // it.postContainerUpdateIfNeeded() | ||
| // } |
Comment on lines
+346
to
+349
| // containerUpdateCoordinator.let { | ||
| // it.invalidateAll() | ||
| // it.postContainerUpdateIfNeeded() | ||
| // } |
| isSelectedTabInvalidated = true | ||
| isBottomNavigationMenuInvalidated = true | ||
| } | ||
| } No newline at end of file |
Comment on lines
+69
to
+75
| // override fun setNavState(view: TabsHost, value: ReadableMap?) { | ||
| // val navStateMap = requireNotNull(value) { "[RNScreens] NavState must not be nullish" } | ||
| // val selectedScreenKey = requireNotNull(navStateMap.getString("selectedScreenKey")) | ||
| // val provenance = requireNotNull(navStateMap.getInt("provenance")) | ||
| // // view.setNavStateFromJS(TabsHost.NavState(selectedScreenKey, provenance)) | ||
| // } | ||
| // |
Member
Author
|
I'll tackle linting & other smaller cleanup problems later, in follow-up PR. |
t0maboro
reviewed
Mar 24, 2026
Comment on lines
+130
to
+132
| post { | ||
| performContainerUpdateIfNeeded() | ||
| } |
Contributor
There was a problem hiding this comment.
nit: this code is repeated 3 times across the whole file
| } | ||
|
|
||
| private fun updateBottomNavigationViewAppearance() { | ||
| RNSLog.d(TabsHost.Companion.TAG, "updateBottomNavigationViewAppearance") |
Contributor
There was a problem hiding this comment.
nit: Should we use TabsHost's TAG outside TabsHost?
Member
Author
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
I'm seeing 1 possible leftover
private fun onMenuItemSelected(item: MenuItem): Boolean {
RNSLog.d(TabsHost.Companion.TAG, "Item selected $item")
t0maboro
approved these changes
Mar 25, 2026
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
This PR splits
TabsHostnative class on Android into two:TabsHostandTabsContainer.This follows RFC-1028
Changes
I've moved all functionalities related to:
(and possibly more) to
TasbContainer.TabsHostis now only a source of configuration, layout andTabsScreens.Notably, I've left the logic related to
layoutCallbackand layoutrefresh inside
TabsHost. I'm not sure where this logic should live.In the future we might to move it from there. One thing to note here is
that
TabsContainerrelies on it - now via call tothis.requestLayout, which calls the overridden function on theTabsHost(view parent).For the transition period, I also pass to
TabsContainera reference toTabsHost. This obviouslycreates circular dependency, which is never resolved, but as I've learned recently from @kligarski
it shouldn't be a problem for the GC. This will be changed later, when I
refactor communication patterns.
Please don't mind commented out code / leftovers in this commit. I
intend to clean them up later, in follow up commits.
Test plan
Regarding functionality - ideally everything should work just as before.
I don't want to perform full manual scan right now, as more "big"
changes are planned before we land this one. I've only asserted that
basic functionality works as intended on
TestBottomTabs.