Repository navigation
feat: add per-tab icon sizing #608
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?
Changes from all commits
1334a22
b2ea12e
132e513
398b6bc
118d508
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| 'react-native-bottom-tabs': minor | ||
| '@bottom-tabs/react-navigation': minor | ||
| --- | ||
|
|
||
| Add a per-screen option for customizing tab image icon size. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import TabView, { SceneMap } from 'react-native-bottom-tabs'; | ||
| import { useState } from 'react'; | ||
| import { Article } from '../Screens/Article'; | ||
| import { Albums } from '../Screens/Albums'; | ||
| import { Contacts } from '../Screens/Contacts'; | ||
| import { Chat } from '../Screens/Chat'; | ||
|
|
||
| const renderScene = SceneMap({ | ||
| article: Article, | ||
| albums: Albums, | ||
| contacts: Contacts, | ||
| chat: Chat, | ||
| }); | ||
|
|
||
| export default function IconSizes() { | ||
| const [index, setIndex] = useState(0); | ||
| const [routes] = useState([ | ||
| { | ||
| key: 'article', | ||
| title: 'Default', | ||
| focusedIcon: require('../../assets/icons/article_dark.png'), | ||
| }, | ||
| { | ||
| key: 'albums', | ||
| title: 'Size 20', | ||
| focusedIcon: require('../../assets/icons/grid_dark.png'), | ||
| iconSize: 20, | ||
| }, | ||
| { | ||
| key: 'contacts', | ||
| title: 'Size 32', | ||
| focusedIcon: require('../../assets/icons/person_dark.png'), | ||
| iconSize: 32, | ||
| }, | ||
| { | ||
| key: 'chat', | ||
| title: 'Size 40', | ||
| focusedIcon: require('../../assets/icons/chat_dark.png'), | ||
| iconSize: 40, | ||
| }, | ||
| ]); | ||
|
|
||
| return ( | ||
| <TabView | ||
| sidebarAdaptable | ||
| labeled | ||
| navigationState={{ index, routes }} | ||
| onIndexChange={setIndex} | ||
| renderScene={renderScene} | ||
| /> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,10 +37,14 @@ import com.facebook.react.common.assets.ReactFontManager | |
| import com.facebook.react.modules.core.ReactChoreographer | ||
| import com.facebook.react.views.text.ReactTypefaceUtils | ||
| import com.google.android.material.bottomnavigation.BottomNavigationView | ||
| import com.google.android.material.navigation.NavigationBarItemView | ||
| import com.google.android.material.navigation.NavigationBarView.LABEL_VISIBILITY_AUTO | ||
| import com.google.android.material.navigation.NavigationBarView.LABEL_VISIBILITY_LABELED | ||
| import com.google.android.material.navigation.NavigationBarView.LABEL_VISIBILITY_UNLABELED | ||
| import com.google.android.material.transition.platform.MaterialFadeThrough | ||
| import kotlin.math.roundToInt | ||
|
|
||
| private data class DrawableCacheKey(val source: ImageSource, val size: Int) | ||
|
|
||
| class ExtendedBottomNavigationView(context: Context) : BottomNavigationView(context) { | ||
| override fun getMaxItemCount(): Int { | ||
|
|
@@ -59,7 +63,8 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| var disablePageAnimations = false | ||
| var items: MutableList<TabInfo> = mutableListOf() | ||
| private val iconSources: MutableMap<Int, ImageSource> = mutableMapOf() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · Android drawable cache is keyed per size and never evicted
Suggested change Bound the cache: drop entries for a source when its size changes (per-source map holding the last size only, or an LRU with a small maximum), and clear it in DetailsReachable case: a tab whose size is driven by state — persona: Reply Useful? Reply |
||
| private val drawableCache: MutableMap<ImageSource, Drawable> = mutableMapOf() | ||
| private var iconSizes: List<Double> = emptyList() | ||
| private val drawableCache: MutableMap<DrawableCacheKey, Drawable> = mutableMapOf() | ||
|
|
||
| private var isLayoutEnqueued = false | ||
| private var selectedItem: String? = null | ||
|
|
@@ -240,7 +245,10 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| } | ||
| } | ||
|
|
||
| fun updateItems(items: MutableList<TabInfo>) { | ||
| fun updateItems(newItems: MutableList<TabInfo>) { | ||
| val items = newItems.mapIndexed { index, item -> | ||
| item.copy(iconSize = iconSizes.getOrNull(index)?.takeIf { it > 0 }) | ||
| }.toMutableList() | ||
| // If an item got removed, let's re-add all items | ||
| if (items.size < this.items.size) { | ||
| bottomNavigation.menu.clear() | ||
|
|
@@ -255,9 +263,9 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| menuItem.isVisible = !item.hidden | ||
| updateIconTintMode(menuItem, item) | ||
| if (iconSources.containsKey(index)) { | ||
| getDrawable(iconSources[index]!!) { | ||
| menuItem.icon = it | ||
| updateIconTintMode(menuItem, item) | ||
| val iconSizePx = getIconSize(item) | ||
| getDrawable(iconSources[index]!!, iconSizePx) { | ||
| applyIcon(menuItem, index, iconSizePx, it) | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -288,6 +296,8 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
|
|
||
| view.findViewById<View>(com.google.android.material.R.id.navigation_bar_item_content_container) | ||
| ?.setTabTestID(item.testID) | ||
|
|
||
| updateIconSize(menuItem, item) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -298,6 +308,11 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| } | ||
| } | ||
|
|
||
| fun setIconSizes(iconSizes: List<Double>) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · Android: setIconSizes runs a full updateItems pass on every prop update, re-enqueuing every icon load and every badge
Suggested change Skip the reload when the incoming iconSizes array equals the stored one, and/or avoid re-creating the default getIconSize closure on every render so the memo stays stable. Detailspersona: Reply Useful? Reply |
||
| this.iconSizes = iconSizes | ||
| updateItems(items) | ||
| } | ||
|
|
||
| private fun getOrCreateItem(index: Int, title: String): MenuItem { | ||
| return bottomNavigation.menu.findItem(index) ?: bottomNavigation.menu.add(0, index, 0, title) | ||
| } | ||
|
|
@@ -344,11 +359,9 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
|
|
||
| // Update existing item if exists. | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · Android: Open ledger item, stated at the strength the code supports. Suggested change In Detailspersona: Reply Useful? Reply |
||
| bottomNavigation.menu.findItem(idx)?.let { menuItem -> | ||
| getDrawable(imageSource) { | ||
| menuItem.icon = it | ||
| items.getOrNull(idx)?.let { item -> | ||
| updateIconTintMode(menuItem, item) | ||
| } | ||
| val iconSizePx = getIconSize(items.getOrNull(idx)) | ||
| getDrawable(imageSource, iconSizePx) { | ||
| applyIcon(menuItem, idx, iconSizePx, it) | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -374,12 +387,16 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| } | ||
|
|
||
| @SuppressLint("CheckResult") | ||
| private fun getDrawable(imageSource: ImageSource, onDrawableReady: (Drawable?) -> Unit) { | ||
| drawableCache[imageSource]?.let { | ||
| private fun getDrawable( | ||
| imageSource: ImageSource, | ||
| iconSizePx: Int, | ||
| onDrawableReady: (Drawable?) -> Unit | ||
| ) { | ||
| val cacheKey = DrawableCacheKey(imageSource, iconSizePx) | ||
| drawableCache[cacheKey]?.let { | ||
| onDrawableReady(it) | ||
| return | ||
| } | ||
| val iconSizePx = bottomNavigation.itemIconSize | ||
| val request = ImageRequest.Builder(context) | ||
| .data(imageSource.getUri(context)) | ||
| .size(CoilSize(iconSizePx, iconSizePx)) | ||
|
|
@@ -388,7 +405,7 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| .target { drawable -> | ||
| post { | ||
| val stateDrawable = drawable.asDrawable(context.resources) | ||
| drawableCache[imageSource] = stateDrawable | ||
| drawableCache[cacheKey] = stateDrawable | ||
| onDrawableReady(stateDrawable) | ||
| } | ||
| } | ||
|
|
@@ -402,6 +419,34 @@ class ReactBottomNavigationView(context: Context) : LinearLayout(context) { | |
| imageLoader.enqueue(request) | ||
| } | ||
|
|
||
| private fun getIconSize(item: TabInfo?): Int { | ||
| val iconSize = item?.iconSize ?: return bottomNavigation.itemIconSize | ||
| return TypedValue.applyDimension( | ||
| TypedValue.COMPLEX_UNIT_DIP, | ||
| iconSize.toFloat(), | ||
| resources.displayMetrics | ||
| ).roundToInt() | ||
| } | ||
|
|
||
| private fun applyIcon(menuItem: MenuItem, index: Int, iconSizePx: Int, drawable: Drawable?) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Major · Android: icon scale is applied to only part of the drawable cache after an iconSize change
Suggested change In updateItems(), drop the cache entries for the icon sources whose resolved size changed (or clear drawableCache wholesale) so every visible item has a request for its current size, instead of relying on the "a request is already in flight" assumption in applyIcon. Detailspersona: Reply Useful? Reply |
||
| // Icons load asynchronously, so read the latest item instead of the one captured at request time. | ||
| val item = items.getOrNull(index) | ||
| // The icon size changed while loading, a request for the new size is already in flight. | ||
| if (getIconSize(item) != iconSizePx) return | ||
|
|
||
| menuItem.icon = drawable | ||
| item?.let { | ||
| updateIconTintMode(menuItem, it) | ||
| updateIconSize(menuItem, it) | ||
| } | ||
| } | ||
|
|
||
| private fun updateIconSize(menuItem: MenuItem, item: TabInfo) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · Android: per-tab size relies on Material's restricted NavigationBarItemView class
Suggested change Replace the NavigationBarItemView#setIconSize call with a public Material API for per-item icon size (or a drawable-level resize) and note the dependency requirement in the README/changeset if the internal class has to stay. DetailsAlso raised hereAlso raised at this location by cpp: Android: per-tab sizing depends on Material's internal NavigationBarItemView.setIconSize (RestrictedApi risk + crash on older Material) — This PR reaches into Material's internals to size a single tab icon: persona: Reply Useful? Reply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · Android: updateIconSize()/icon layout is never reset when a tab has no iconSize (and after a configuration change)
DetailsAlso raised hereAlso raised at this location by senior-developer: Android never resets a tab's icon size once one has been applied — persona: Reply Useful? Reply There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor ·
Suggested change Make the icon-size application not depend on the item view existing at that instant: keep the desired px size per index and (re)apply it from a layout callback / Detailspersona: Reply Useful? Reply |
||
| val itemView = bottomNavigation.findViewById<NavigationBarItemView>(menuItem.itemId) | ||
| ?: return | ||
| itemView.setIconSize(getIconSize(item)) | ||
| } | ||
|
|
||
| fun setBarTintColor(color: Int?) { | ||
| // Set the color, either using the active background color or a default color. | ||
| val backgroundColor = | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ data class TabInfo( | |
| val badgeBackgroundColor: Int?, | ||
| val badgeTextColor: Int?, | ||
| val activeTintColor: Int?, | ||
| val iconSize: Double?, | ||
| val iconRenderingMode: String?, | ||
| val hidden: Boolean, | ||
| val testID: String? | ||
|
|
@@ -105,6 +106,7 @@ class RCTTabViewManager(context: ReactApplicationContext) : | |
| badgeBackgroundColor = if (item.hasKey("badgeBackgroundColor")) item.getInt("badgeBackgroundColor") else null, | ||
| badgeTextColor = if (item.hasKey("badgeTextColor")) item.getInt("badgeTextColor") else null, | ||
| activeTintColor = if (item.hasKey("activeTintColor")) item.getInt("activeTintColor") else null, | ||
| iconSize = null, | ||
| iconRenderingMode = if (item.hasKey("iconRenderingMode")) item.getString("iconRenderingMode") else null, | ||
| hidden = if (item.hasKey("hidden")) item.getBoolean("hidden") else false, | ||
| testID = item.getString("testID") | ||
|
|
@@ -126,6 +128,16 @@ class RCTTabViewManager(context: ReactApplicationContext) : | |
| view.setIcons(value) | ||
| } | ||
|
|
||
| override fun setIconSizes(view: ReactBottomNavigationView?, value: ReadableArray?) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 Major · Android: setIconSizes(null) wipes every per-tab iconSize, including route.iconSize
Suggested change Treat a null iconSizes array as "no sizes provided" and skip the update (or keep the previous array), so that clearing the prop does not silently drop sizes; if clearing is intended, note it in the changeset. Details
persona: Reply Useful? Reply |
||
| view?.setIconSizes( | ||
| if (value == null) { | ||
| emptyList() | ||
| } else { | ||
| (0 until value.size()).map { value.getDouble(it) } | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| override fun setFocusedIcons(view: ReactBottomNavigationView?, value: ReadableArray?) { | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -38,6 +38,7 @@ | |
| @property(nonatomic, strong) NSNumber *fontSize; | ||
| @property(nonatomic) BOOL tabBarHidden; | ||
| @property(nonatomic, copy) NSArray *itemsData; | ||
| @property(nonatomic, copy) NSArray<NSNumber *> *iconSizes; | ||
| - (void)setImageLoader:(RCTImageLoader *)imageLoader; | ||
| - (void)insertChild:(UIView *)child atIndex:(NSInteger)index; | ||
| - (void)removeChildAtIndex:(NSInteger)index; | ||
|
|
@@ -53,6 +54,7 @@ FOUNDATION_EXPORT NSObject *RNCCreateBottomAccessoryProvider(id<RNCBottomAccesso | |
| sfSymbol:(NSString *)sfSymbol | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Minor · RNCTabInfo createWithKey:...: gains a required iconSize: argument — source-breaking for SwiftPM consumers, undocumented
Suggested change Mention in the changeset (and, if the SwiftPM bridge is public API, in the upgrade/migration docs) that Detailspersona: Reply Useful? Reply |
||
| focusedSfSymbol:(NSString *)focusedSfSymbol | ||
| activeTintColor:(UIColor *)activeTintColor | ||
| iconSize:(NSNumber *)iconSize | ||
| iconRenderingMode:(NSString *)iconRenderingMode | ||
| hidden:(BOOL)hidden | ||
| testID:(NSString *)testID | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,14 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & | |
| const auto &oldViewProps = *std::static_pointer_cast<RNCTabViewProps const>(_props); | ||
| const auto &newViewProps = *std::static_pointer_cast<RNCTabViewProps const>(props); | ||
|
|
||
| if (oldViewProps.iconSizes != newViewProps.iconSizes) { | ||
| NSMutableArray<NSNumber *> *iconSizes = [NSMutableArray arrayWithCapacity:newViewProps.iconSizes.size()]; | ||
| for (const auto iconSize : newViewProps.iconSizes) { | ||
| [iconSizes addObject:@(iconSize)]; | ||
| } | ||
| _tabViewProvider.iconSizes = iconSizes; | ||
| } | ||
|
|
||
| if (oldViewProps.items != newViewProps.items) { | ||
| _tabViewProvider.itemsData = convertItemsToArray(newViewProps.items); | ||
| } | ||
|
|
@@ -237,6 +245,7 @@ - (void)updateProps:(Props::Shared const &)props oldProps:(Props::Shared const & | |
| sfSymbol:RCTNSStringFromStringNilIfEmpty(item.sfSymbol) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Nit · The new plumbing hardcodes the value instead of reading it from the prop it sits next to: Suggested change Prefer carrying Detailspersona: Reply Useful? Reply |
||
| focusedSfSymbol:RCTNSStringFromStringNilIfEmpty(item.focusedSfSymbol) | ||
| activeTintColor:RCTUIColorFromSharedColor(item.activeTintColor) | ||
| iconSize:nil | ||
| iconRenderingMode:RCTNSStringFromStringNilIfEmpty(item.iconRenderingMode) | ||
| hidden:item.hidden | ||
| testID:RCTNSStringFromStringNilIfEmpty(item.testID) | ||
|
|
||
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.
🟡 Minor · Android: duplicate drawables per icon size, and every size variant is kept for the view's lifetime
DrawableCacheKeyputs the resolved pixel size into the key (private data class DrawableCacheKey(val source: ImageSource, val size: Int)), anddrawableCacheis never evicted. A tab bar whoseiconSizeschange over its lifetime — e.g. a size driven by state (the newgetIconSize/tabBarIconSizeis re-evaluated on every render, andupdateItemsre-runs whenever items or sizes change) — accumulates one decoded Drawable per (icon, size) pair for as long as the view lives. With 5 tabs and a handful of sizes that is several full bitmaps held indrawableCachein addition to Coil's own memory cache.Details
The same key change also defeats the cross-tab reuse the old
Map<ImageSource, Drawable>provided: two tabs that use the same asset at the same size each got one Drawable before, and still share via the key, but the same asset at 20/32/40 is now decoded and retained three times.updateItemsalready posts work on every call, so add a size-keyed eviction (e.g. keep only the size currently requested per source) or cleardrawableCachewhen the sizes change.Also raised here
Also raised at this location by maintainability: The drawable cache key gained
size, so the cache is now bounded by distinct (source, size) pairs instead of distinc… — The drawable cache key gainedsize, so the cache is now bounded by distinct (source, size) pairs instead of distinct sources, and nothing evicts it: resizing the same tabs through several sizes (a state-driven or animated tabBarIconSize, or route churn where a size moves between indexes) keeps every rendered variant alive for the life of the view, and a size derived from layout can take many values. Before this change the worst case was one drawable per icon source. Consider clearing the entries for a source when its size changes, or bucketing the size in the key.persona:
cpp,maintainabilityReply
@apex-sentinel-staging note ...to add context, or@apex-sentinel-staging pivot ...if this is wrong — I will re-check the cited code.Useful? Reply
@apex-sentinel-staging like, or@apex-sentinel-staging dislike <why>if it is not.