Skip to content
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

fix: no longer spread key prop on BottomNavigationBar - resolves #4401 #4494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

nick42d
Copy link

@nick42d nick42d commented Sep 12, 2024

Motivation

Resolves #4401 with suggested fix from @CommanderRedYT

Related issue

#4401

Test plan

Tested following contribution guidelines https://github.com/callstack/react-native-paper/blob/main/CONTRIBUTING.md

@callstack-bot
Copy link

Hey @nick42d, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@nick42d nick42d mentioned this pull request Sep 21, 2024
@hello-ccchen
Copy link

any update?

@prageeth
Copy link

We are waiting for this too, would be great to have this merged and included in the next release. Thanks

@skoolaidl
Copy link

Is this going to get merged?

@shuffledex
Copy link

come on guys! launch a new release asap!

@@ -360,7 +360,9 @@ const BottomNavigationBar = <Route extends BaseRoute>({
navigationState,
renderIcon,
renderLabel,
renderTouchable = (props: TouchableProps<Route>) => <Touchable {...props} />,
renderTouchable = ({ key, ...props }: TouchableProps<Route>) => (
<Touchable key={key} {...props} />
Copy link

@elibroftw elibroftw Dec 1, 2024

Choose a reason for hiding this comment

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

'key' is specified more than once, so this usage will be overwritten.

I suggest keeping it at one line

renderTouchable = (props: TouchableProps<Route>) => <Touchable {...props} key={props.key} />

Choose a reason for hiding this comment

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

You sure this works? props will still contain the key, so not sure if react will stop complaining when it is not split

Copy link

@elibroftw elibroftw Dec 1, 2024

Choose a reason for hiding this comment

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

Have you never added duplicate prop to a component? The last prop takes precedence. Also the error is a Typescript error visible in an IDE. I'm also already using this code in production since I didn't want the ripples that used to be present in the bottom tabs (I'm not sure if the ripples are removed in more recent versions):

renderTouchable={props => (
            <TouchableWithoutFeedback {...props} key={props.key} >
              <View {...props} key={props.key} />
            </TouchableWithoutFeedback>
          )}

Choose a reason for hiding this comment

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

Have you never added duplicate prop to a component? The last prop takes precedence. Also the error is a Typescript error visible in an IDE. I'm also already using this code in production since I didn't want the ripples that used to be present in the bottom tabs (I'm not sure if the ripples are removed in more recent versions):

renderTouchable={props => (
            <TouchableWithoutFeedback {...props} key={props.key} >
              <View {...props} key={props.key} />
            </TouchableWithoutFeedback>
          )}

it copy it and pass it work thian k u

Copy link
Author

Choose a reason for hiding this comment

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

'key' is specified more than once, so this usage will be overwritten.

I suggest keeping it at one line

renderTouchable = (props: TouchableProps<Route>) => <Touchable {...props} key={props.key} />

This seems like a bit of a nitpick to me. Is this required?

Copy link

Choose a reason for hiding this comment

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

Surely it is better to destructure the props and avoid any potential duplicate keys, as the initial suggestion suggests, who cares if it spreads onto 2 lines rather than one, it's just source code and makes no difference to the published and production code.

Choose a reason for hiding this comment

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

I have tested this with patch-package and it works and gets rid of the error. idk what y'all are going on about but this looks great and we need to merge it so we don't have to patch the library

here is exactly my patch-package for proof:

diff --git a/node_modules/react-native-paper/src/components/BottomNavigation/BottomNavigationBar.tsx b/node_modules/react-native-paper/src/components/BottomNavigation/BottomNavigationBar.tsx
index 0bfe303..0195d70 100644
--- a/node_modules/react-native-paper/src/components/BottomNavigation/BottomNavigationBar.tsx
+++ b/node_modules/react-native-paper/src/components/BottomNavigation/BottomNavigationBar.tsx
@@ -360,7 +360,9 @@ const BottomNavigationBar = <Route extends BaseRoute>({
   navigationState,
   renderIcon,
   renderLabel,
-  renderTouchable = (props: TouchableProps<Route>) => <Touchable {...props} />,
+  renderTouchable = ({key, ...props}: TouchableProps<Route> & { key: string }) => (
+    <TouchableRipple key={key} {...props} />
+  ),
   getLabelText = ({ route }: { route: Route }) => route.title,
   getBadge = ({ route }: { route: Route }) => route.badge,
   getColor = ({ route }: { route: Route }) => route.color,

@hello-ccchen
Copy link

any update? 🥹

@lourencorodrigo
Copy link

Please guys. Launch a new release with this fix.

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.

Future chore - BottomNavigation: React 18.3 to warn about spreading key props in JSX