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.

Sign up for GitHub

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
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/components/BottomNavigation/BottomNavigationBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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} />

Copy link

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 •
edited
Loading

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>
          )}

Copy link

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.

Copy link

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,

),
getLabelText = ({ route }: { route: Route }) => route.title,
getBadge = ({ route }: { route: Route }) => route.badge,
getColor = ({ route }: { route: Route }) => route.color,
Expand Down