Skip to content

Conversation

johankasperi
Copy link

@johankasperi johankasperi commented Jun 27, 2025

Description

The current implementation of headerLeft and headerRight adds a react view as a custom view in a UIBarButtonItem. This implementation is sufficient at most times but I believe we can achieve greater "native feel" if the native stack has an protocol for adding actual UIBarButtonItems in the header. As the UIBarButtonItems has properties and features that can be difficult to mimic with a react view.

Also with the introduction of iOS 26, using only custom views in UIBarButtonItem presents some limitations. Mainly that the adaptive tint color (based on the underlying view) is not working on a UIBarButtonItem with a custom view as demonstrated below under "Screenshots".

Changes

This PR adds the properties headerRightItems and headerLeftItems on the native stack Screen that makes it possible to add one or several UIBarButtonItem to the right/left of the header and/or functions returning a React Node. Most of the features of the UIBarButtonItem is supported (see either "Bar Button Items" in the example apps or the type definition).

Screenshots / GIFs

Non adaptive tint color when using old property headerRight on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.31.10.mov

Adaptive tint color when using new property headerRightBarButtonItems on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.28.30.mov

UIBarButtonItem with style "prominent" on iOS 26

Simulator Screenshot - iPhone 16 Pro - 2025-06-27 at 16 28 39

UIBarButtonItem with UIMenu on iOS 26

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-06-27.at.16.29.02.mov

Test code and steps to reproduce

I've created a screen named "Bar Button Items" in the example app that showcases all of the proposed features.

Checklist

@johankasperi
Copy link
Author

johankasperi commented Jun 27, 2025

Related PR in react-navigation: react-navigation/react-navigation#12657

@Pnlvfx
Copy link

Pnlvfx commented Jul 2, 2025

i like this

@kkafar
Copy link
Member

kkafar commented Jul 18, 2025

Hey! This is really promising. We're working currently on support for iOS 26 etc. and we're doing general overhaul of the lib. As of now, we're not sure what will be the API shape & scope, therefore I won't review it for now. Once we settle the internal discussion I'll revisit your implementation. I'm sure we'll be able to land it in some form.

@johankasperi
Copy link
Author

johankasperi commented Aug 4, 2025

Hey! This is really promising. We're working currently on support for iOS 26 etc. and we're doing general overhaul of the lib. As of now, we're not sure what will be the API shape & scope, therefore I won't review it for now. Once we settle the internal discussion I'll revisit your implementation. I'm sure we'll be able to land it in some form.

I'm really glad you liked it! Yeah I saw that you have done a bunch of iOS 26 related PRs and changes to the API so I understand that my suggestion may not match the shape you are targeting. Please let me know if there are any changes I can do right now and I will happily do them asap. I truly believe something like this would be awesome for react-native-screens when iOS 26 is out of beta.

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi
Great stuff. We're looking at your PR and would like to merge the changes soon. Here are some comments from me, and I believe @kkafar wants to look more closely in the following days.

Also, please update the PR with changes from main branch. There could be some conflicts.

@johankasperi johankasperi force-pushed the ios-bar-button-item branch 2 times, most recently from e9b25c0 to f53da7d Compare August 6, 2025 10:00
@johankasperi
Copy link
Author

johankasperi commented Aug 6, 2025

Hi Great stuff. We're looking at your PR and would like to merge the changes soon. Here are some comments from me, and I believe @kkafar wants to look more closely in the following days.

Also, please update the PR with changes from main branch. There could be some conflicts.

Thank you! I've updated the PR with changes from main and resolved all conflicts

@kmichalikk kmichalikk requested a review from kkafar August 7, 2025 04:56
kmichalikk added a commit that referenced this pull request Aug 12, 2025
## Description

From discussion in
#2987 (comment).
We should rename the class into something more specific because it
really does one specific thing.

## Changes

title

## Test code and steps to reproduce

No changes, disabling back button menu should still work. You can modify
BottomTabsTest / Tab4 with `screenOption={{ headerBackButtonMenuEnabled:
false }}`
@kkafar
Copy link
Member

kkafar commented Aug 12, 2025

Hey, I most likely won't find time tomorrow & I'm going for a two week PTO on Thursday. I'll try to review this as one of first things after I'm back from PTO.

:sorry:

@johankasperi
Copy link
Author

Hey, I most likely won't find time tomorrow & I'm going for a two week PTO on Thursday. I'll try to review this as one of first things after I'm back from PTO.

:sorry:

Hey! No worries! Have a nice PTO 😊

@johankasperi
Copy link
Author

@kmichalikk I added support for systemImage (using SF Symbols) in this commit 8981d6a Feel free to take a look or try it out whenever you have the chance. Thanks!

@johankasperi
Copy link
Author

johankasperi commented Aug 19, 2025

@kmichalikk refactored everything a bit in af27795

I realised that users might want to combine native UIBarButtonItems and React Nodes in the header left and right. So renamed headerRightBarButtonItems and headerLeftRightBarButtonItems to headerRightItems and headerLeftItems. These two properties now accepts an array containing either dicts for UIBarButtonItems or functions returning a React Node. The new functionality is shown in the screen "ReactNodeButtonDemo".

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to properly run this probably because of some issues with linking to react-navigation PR - buttons were not displayed at all. I'll try again later. In the meantime I have two small nitpicks.

@johankasperi
Copy link
Author

I wasn't able to properly run this probably because of some issues with linking to react-navigation PR - buttons were not displayed at all. I'll try again later. In the meantime I have two small nitpicks.

My bad! I forgot to push my latest commits to react-navigation. Should be working now

@johankasperi
Copy link
Author

@kmichalikk I added support for hidesSharedBackground: boolean when rendering custom React Elements. Making it possible to show them without the liquid glass effect (see screenshot below). Feel free to have a look and try it out!

Simulator Screenshot - iPhone 16 Pro - 2025-09-02 at 12 05 26

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍
Some nitpick & suggestions below.
I pulled the PR locally and merged main, works fine but found some visual bugs that I suspect are native, but haven't checked that yet.

Comment on lines 147 to 152
#if defined(__IPHONE_OS_VERSION_MAX_ALLOWED) && defined(__IPHONE_26_0) && \
__IPHONE_OS_VERSION_MAX_ALLOWED >= __IPHONE_26_0
if (@available(iOS 26.0, *)) {
self.style = UIBarButtonItemStyleProminent;
}
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For iOS < 26 there is no style set explicitly here; maybe, to be consistent, we should set it to something? And please mention the difference for iOS versions somewhere in the docs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to set it to something else on iOS < 26. I believe it's better to leave it untouched and use the default style of the OS when the prominent style is not available. UIBarButtonItemStylePlain is the default style of iOS < 26.

Copy link
Author

@johankasperi johankasperi Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And about docs, I'm linking to the Apple docs in the JSDoc comments in the type declaration. I'm also mentioning that "Prominent" is only available on iOS 26. https://github.com/johankasperi/react-navigation/blob/668b77545440c51f473bf7e392a6b8b946eec8f9/packages/native-stack/src/types.tsx#L729

Could you point me to where in the docs I should also mention this?

One could also argue that linking to the iOS docs is sufficient, especially during the beta period when stuff might be changed by Apple, since by trying to copy Apples docs to the React Native Screen docs the RNS docs might become dated quickly when Apple are doing changes. But I'm not gonna be the judge of that 😊

@@ -0,0 +1,412 @@
// NOTE: The full native feature set (style, image, menu, etc.) is available, but the TS types in src/types.tsx need to be updated to match. This example uses only the currently typed props (title, icon, onPress, enabled).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we should add some documentation to src/types.tsx, maybe also to GUIDE_FOR_LIBRARY_AUTHORS, especially the different cases when the behavior is different for iOS 26 vs 18. I saw there is more documentation in the PR for react-navigation, but it would be nice to have it also here in screens.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean this interface?

export interface ScreenStackHeaderConfigProps extends ViewProps {

And in GUIDE_FOR_LIBRARY_AUTHORS, do you mean I should add it under this section? https://github.com/software-mansion/react-native-screens/blob/main/guides/GUIDE_FOR_LIBRARY_AUTHORS.md#screenstackheaderconfig

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I was thinking about, It'd be good to have something in the same place where other screens components' props are defined for quick reference when someone new jumps in. IN GUIDE... there is this section "Below is a list of properties that can be set with ScreenStackHeaderConfig component:" and since you are adding new props, we should keep it in sync & write something there. Same for src/types.tsx.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thansk for pointing me to where I should write something. I will do that asap

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the types in this commit 3e2f63d

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added docs in this commit b41acf8

@johankasperi
Copy link
Author

I tested this and it seems to fix several UI issues on iOS 26. Thank you @johankasperi ! 🙏

@kkafar @kmichalikk is there any chance this might be released in the next few days? 😇 We need this to get our app ready for iOS 26 with a deadline in mid September. 🙈

Thank you! Don't know your circumstances for the deadline but wanted to share that you can opt out from the new design with UIDesignRequiresCompatibility in your Info.plist

@joluet
Copy link

joluet commented Sep 5, 2025

Thank you! Don't know your circumstances for the deadline but wanted to share that you can opt out from the new design with UIDesignRequiresCompatibility in your Info.plist

Apple is considering our App for featuring. So, opting out is no an option for us. :/

@johankasperi
Copy link
Author

Thank you! Don't know your circumstances for the deadline but wanted to share that you can opt out from the new design with UIDesignRequiresCompatibility in your Info.plist

Apple is considering our App for featuring. So, opting out is no an option for us. :/

I understand. Congrats for being considered for featuring!

Copy link
Collaborator

@kmichalikk kmichalikk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, great job. I left a comment about the docs, nothing else from me.

@kkafar
Copy link
Member

kkafar commented Sep 8, 2025

I wanna update you, that I'm starting to look at this PR 😅 I can't say however we'll be able to land in in just couple of days, since it's a bit chunky.

Remember that you always have patch-package at your disposal (know that it is kinda clunky solution, but if forced to - may be a way to go).

Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. This looks really good. I have few initial remarks. Please answer them.


Screen.Recording.2025-09-09.at.18.11.38.mov

I wonder what is going on with the shadow beneath the bar button items, that disappears after few seconds. @kligarski is that something you experienced when testing iOS 26?


NSDictionary *imageObj = dict[@"image"];
if (imageObj) {
self.image = [RCTConvert UIImage:imageObj];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're relying here on deprecated RCTConvert. Do you happen to know whether there is alternative or not?

I'll try to suggest alternative (if there is one) when I have a moment to research it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope sorry thats the only way I know to convert a react image to a UIImage

Comment on lines 124 to 125
objc_setAssociatedObject(self, &RNSBarButtonItemIdKey, buttonId, OBJC_ASSOCIATION_COPY_NONATOMIC);
objc_setAssociatedObject(self, &RNSBarButtonItemActionKey, action, OBJC_ASSOCIATION_COPY_NONATOMIC);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we go for ObjC runtime functions instead of regular members? What's the reasoning here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I guess I wanted a quick solution given that the properties was meant to be private for this class and I didn't give it much thought. But you're right, regular instance properties are clearer. I refactored to that in 5d6037b

@kligarski
Copy link
Contributor

I wonder what is going on with the shadow beneath the bar button items, that disappears after few seconds. @kligarski is that something you experienced when testing iOS 26?

I noticed some unusual shadow behavior with the search bar in the header but I did not pay much attention to it yet as there were more critical bugs with the search bar that could've been impacting the shadows.

@kkafar
Copy link
Member

kkafar commented Sep 10, 2025

Okay, I'll create ticket for this to test it out on pure native app, to verify whether this is UIKit issue or something on our end.

@johankasperi
Copy link
Author

johankasperi commented Sep 10, 2025

I wonder what is going on with the shadow beneath the bar button items, that disappears after few seconds. @kligarski is that something you experienced when testing iOS 26?

I haven't noticed that! I almost hope it's a bug in iOS 26 because I haven't written any code (as far as I know) that should affect that. But good catch!

It is happening on the back button in other demo screens and in the News app from apple. So I'm guessing its part of UIKit

Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-09-10.at.22.51.05.mov
Simulator.Screen.Recording.-.iPhone.16.Pro.-.2025-09-10.at.22.52.47.mov

@johankasperi
Copy link
Author

Okay. This looks really good. I have few initial remarks. Please answer them.

Thanks for the review! I think I have adressed all of your remarks. Please let me know if you have any further questions or if there is anything else I should take a look at.

@johankasperi johankasperi requested a review from kkafar September 10, 2025 20:46
Copy link
Member

@kkafar kkafar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My follow-up review. Please answer it. I'll try to help with codechanges & guidance when I have a moment.

More issues I've found listed below 👇🏻 (I'll try to help fixing some of them when I have a moment).

Legend:

⚠️ - non-blocking issue
🚫 - blocking issue


⚠️ No. 1 - Notice that the header title appears only after a delay when using spacing. I haven't observed that in other screens.

I consider this issue to be non-blocking for the PR. Just something to be aware of & that need to be fixed.

This is iOS 18.6

Screen.Recording.2025-09-11.at.12.16.53.mov

⚠️ No.2 - Header

      <Stack.Screen
        name="IconButtonDemo"
        component={IconButtonDemo}
        options={{
          title: 'Icon Button',
          headerLeft: () => (<View style={{ height: 64, width: 128, backgroundColor: 'red' }} />),
          headerRightItems: [
            {
              image: require('../../assets/search_black.png'),
              onPress: () => Alert.alert('Icon pressed'),
            },
          ],
        }}
      />

When I've added headerLeft - it looks terrible on swipe back.

Screen.Recording.2025-09-11.at.12.24.49.mov

🚫 No. 3 - POTENTIALLY BLOCKING

The same recording as in issue no. 2.

There is no back button present when I've specified headerLeft. IIRC there should be back button present. I need to verify whether this is an regression or not.


⚠️ No. 4

      <Stack.Screen name="BackButtonVisibleDemo"
        component={BackButtonVisibleDemo}
        options={{
          title: 'Back Button Visible',
          headerBackVisible: true,
          headerRight: () => (<View style={{ height: 64, width: 128, backgroundColor: 'red' }} />),
          headerLeftItems: [
            {
              // eslint-disable-next-line react/no-unstable-nested-components
              customView: () => <TouchableOpacity onPress={() => Alert.alert('Left React Node')}>
                <Text style={{ color: 'blue' }}>React Node</Text>
              </TouchableOpacity>
            },
            {
              title: "Native",
              onPress: () => Alert.alert('Native button pressed'),
            },
          ],
        }} />
Screen.Recording.2025-09-11.at.12.35.54.mov

Notice that "excessive" items are put inside "more item" and there is no way to access them.


🚫 / ⚠️ No. 5 (haven't made my mind whether it's blocking or not)

      <Stack.Screen
        name="IconSharesBgButtonDemo"
        component={IconSharesBgButtonDemo}
        options={{
          title: 'Icon SharesBackground',
          headerTitle: () => (<Text>Icon SharesBackground VERY LONG TITLE THAT REQUIRES TRUNCATION</Text>),
          headerRightItems: [
            {
              image: require('../../assets/search_black.png'),
              onPress: () => Alert.alert('Icon with sharesBackground pressed'),
              sharesBackground: true,
            },
            {
              image: require('../../assets/search_black.png'),
              onPress: () => Alert.alert('Icon with sharesBackground pressed'),
              sharesBackground: true,
            },
            {
              image: require('../../assets/search_black.png'),
              onPress: () => Alert.alert('Icon with sharesBackground pressed'),
              sharesBackground: false,
            },
            {
              image: require('../../assets/search_black.png'),
              hidesSharedBackground: true,
              onPress: () => Alert.alert('Icon with sharesBackground false pressed'),
            },
          ],
        }}
      />
issue no. 5

Please notice that Yoga-layouted text wont' be truncated properly at all.


Other remarks:

✅ I've validated that when both headerRight & headerRightItems are present the headerRightItems takes precedence.
✅ I've validated that when both headerLeft & headerLeftItems are present the headerLeftItems takes precedence.
🚫 Above two interactions are desired but they should be documented in GUIDE_FOR_LIBRARY_AUTHORS and in types. Here and in react-navigation.

Comment on lines +544 to +600
### `headerLeftItems` / `headerRightItems` (iOS only)

An array of objects describing native bar button items to display on the left or right side of the header. Each item can be:

- A button with a title, icon, and action
- A menu with multiple actions
- A spacing item to adjust layout
- A object for configuration of the `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView`

#### The button and menu items support:

`title: string` — Title of the button.

`titleStyle?: object` — Style for the button title.

`image?: ImageRequireSource` — Image source for the button icon.

`sfSymbolName?: string` - SF Symbol for the button icon.

`style?: 'Plain' | 'Done' | 'Prominent'` — The style of the item. 'Prominent' only available for iOS 26+.

`tintColor?: ColorValue` — Tint color for the button. Will pick the tint color of the header if not set.

`hidden?: boolean` — Whether the item is hidden (iOS 16+).

`enabled?: boolean` — Whether the item is enabled.

`width?: number` — Width of the item (iOS 18-).

`possibleTitles?: string[]` — Possible titles for the item.

`hidesSharedBackground?: boolean` — Hide shared background (iOS 26+).

`sharesBackground?: boolean` — Share background with other items (iOS 26+).

`identifier?: string` — Identifier for matching items across transitions (iOS 26+).

`badge: object` - Badge configuration for the button (iOS 26+).

#### The button with an action also supports:

`selected?: boolean` — Whether the button is selected.

`changesSelectionAsPrimaryAction?: boolean` — Whether selection changes as a primary action (iOS 15+).

#### The button with a menu also support:

`menu?: Array<object>` — Menu actions for the item.

#### The spacing item supports:

`spacing?: number` — Fixed space between items. The numeric value is only supported on iOS 18-

#### The configuration item of the custom views supports:

`hidesSharedBackground?: boolean` - Hide shared background (iOS 26+).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is missing example configuration. We should add one, that uses all the props. You can copy configuration from the example you've already written.

Comment on lines +544 to +600
### `headerLeftItems` / `headerRightItems` (iOS only)

An array of objects describing native bar button items to display on the left or right side of the header. Each item can be:

- A button with a title, icon, and action
- A menu with multiple actions
- A spacing item to adjust layout
- A object for configuration of the `ScreenStackHeaderRightView` or `ScreenStackHeaderLeftView`

#### The button and menu items support:

`title: string` — Title of the button.

`titleStyle?: object` — Style for the button title.

`image?: ImageRequireSource` — Image source for the button icon.

`sfSymbolName?: string` - SF Symbol for the button icon.

`style?: 'Plain' | 'Done' | 'Prominent'` — The style of the item. 'Prominent' only available for iOS 26+.

`tintColor?: ColorValue` — Tint color for the button. Will pick the tint color of the header if not set.

`hidden?: boolean` — Whether the item is hidden (iOS 16+).

`enabled?: boolean` — Whether the item is enabled.

`width?: number` — Width of the item (iOS 18-).

`possibleTitles?: string[]` — Possible titles for the item.

`hidesSharedBackground?: boolean` — Hide shared background (iOS 26+).

`sharesBackground?: boolean` — Share background with other items (iOS 26+).

`identifier?: string` — Identifier for matching items across transitions (iOS 26+).

`badge: object` - Badge configuration for the button (iOS 26+).

#### The button with an action also supports:

`selected?: boolean` — Whether the button is selected.

`changesSelectionAsPrimaryAction?: boolean` — Whether selection changes as a primary action (iOS 15+).

#### The button with a menu also support:

`menu?: Array<object>` — Menu actions for the item.

#### The spacing item supports:

`spacing?: number` — Fixed space between items. The numeric value is only supported on iOS 18-

#### The configuration item of the custom views supports:

`hidesSharedBackground?: boolean` - Hide shared background (iOS 26+).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're also missing references the the Apple docs, the same ones that you added to the types (I'm sure I've seen it somewhere).


`sfSymbolName?: string` - SF Symbol for the button icon.

`style?: 'Plain' | 'Done' | 'Prominent'` — The style of the item. 'Prominent' only available for iOS 26+.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
`style?: 'Plain' | 'Done' | 'Prominent'` — The style of the item. 'Prominent' only available for iOS 26+.
`style?: 'plain' | 'done' | 'prominent'` — The style of the item. 'prominent' only available for iOS 26+.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's not only a typo, but the union type is really defined this way - then let's change it in both places.


`width?: number` — Width of the item (iOS 18-).

`possibleTitles?: string[]` — Possible titles for the item.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is this being used instead of regular title? Depending on available space?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should elaborate a bit more here, please


`sharesBackground?: boolean` — Share background with other items (iOS 26+).

`identifier?: string` — Identifier for matching items across transitions (iOS 26+).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it supported? Can we add example showcasing this? If not, this should not be part of the API.

NSArray<UIBarButtonItem *> *currentItems = navitem.rightBarButtonItems ?: @[];
NSMutableArray<UIBarButtonItem *> *mutableItems = [currentItems mutableCopy];
[mutableItems addObject:buttonItem];
navitem.rightBarButtonItems = [mutableItems copy];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thing here

@@ -873,6 +890,71 @@ - (void)applySemanticContentAttributeIfNeededToNavCtrl:(UINavigationController *
}
}

- (NSArray<UIBarButtonItem *> *)barButtonItemsFromDictionaries:(NSArray<NSDictionary<NSString *, id> *> *)dicts
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- (NSArray<UIBarButtonItem *> *)barButtonItemsFromDictionaries:(NSArray<NSDictionary<NSString *, id> *> *)dicts
- (NSArray<UIBarButtonItem *> *)barButtonItemsFromConfigs:(NSArray<NSDictionary<NSString *, id> *> *)dicts

Sorry, should have noticed that last time.

Comment on lines -961 to -962
navitem.leftBarButtonItem.customView = snapshot;
break;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need a reasoning why this has been removed here. As this code was patching some issues with header items disappearing during back transition. Is this now patched somehow else? Why do we need to modify this?

Comment on lines +80 to +81
headerLeftBarButtonItems?: UnsafeMixed[];
headerRightBarButtonItems?: UnsafeMixed[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be aware of: single UnsafeMixed translated to folly::dynamic under the hood is at least 64 bytes. Having a long array of these, can be really memory intensive.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is implementation detail of the library, this version is ok now. We can always improve it in followups on as needed basis.

Comment on lines +613 to +632
/**
* Array of iOS native UIBarButtomItem or functions
* that returns a React Element to the left side of the header.
*
* @platform ios
*/
headerLeftBarButtonItems?: (
| HeaderBarButtonItem
| HeaderBarButtonItemWithCustomView
)[];
/**
* Array of iOS native UIBarButtomItem or functions
* that returns a React Element to the right side of the header.
*
* @platform ios
*/
headerRightBarButtonItems?: (
| HeaderBarButtonItem
| HeaderBarButtonItemWithCustomView
)[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to document here, that these override headerLeft & headerRight respectively. Additionally, we need to document on headerLeft & headerRight that they get overrided by these.

@JustJoostNL
Copy link

This PR seems promising! Can't wait! Any ETA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants