-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix: Each child in a list should have a unique "key" prop #5443
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
Conversation
|
Hey @GleidsonDaniel, I have made some fixes to the mentioned issue. When you have a moment, would you kindly review the changes I have made and provide any feedback? Please let me know if you have any questions or need any clarification on the updates. Thank you for your time and consideration. Regards, |
| {fields.map(field => ( | ||
| <Text style={[styles.text, styles.field, { color: themes[theme].bodyText }]}>{parser.text(field)}</Text> | ||
| {fields.map((field, index) => ( | ||
| <Text key={index} style={[styles.text, styles.field, { color: themes[theme].bodyText }]}> |
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.
https://react.dev/learn/rendering-lists#why-does-react-need-keys
See pitfall section
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.
Okay sir. Thanks for pointing that out.
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.
This issue has not yet been resolved.
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.
Sir I find this one trivial as the field is of type any which means that using this as the key it can cause major bugs in future so that's the reason I have made use of the index here.
I would love to know suggestions.
0f774bc to
2cccebe
Compare
|
@GleidsonDaniel Sir I have made the necessary changes and reviewed the review. You can have a look. Thanks for your time |
| {fields.map(field => ( | ||
| <Text style={[styles.text, styles.field, { color: themes[theme].bodyText }]}>{parser.text(field)}</Text> | ||
| {fields.map((field, index) => ( | ||
| <Text key={index} style={[styles.text, styles.field, { color: themes[theme].bodyText }]}> |
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.
This issue has not yet been resolved.
| {avatarSuggestions.slice(0, 7).map(item => ( | ||
| <AvatarSuggestionItem item={item} testID={`${item?.service}-avatar-suggestion`} onPress={onPress} /> | ||
| {avatarSuggestions.slice(0, 7).map((item, index) => ( | ||
| <AvatarSuggestionItem key={index} item={item} testID={`${item?.service}-avatar-suggestion`} onPress={onPress} /> |
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.
Same as above
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.
Sir, now I have made use of the url of the avatar instead of indices as key as it was the most feasibly unique thing that differentiates each avatar better.
Let me know if you disagree with this.
preeesha
left a comment
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.
I have reviewed the suggestions and left the comments in the respective conversation.
Kindly provide your views on them.
|
Hey @preeesha, are you still working on this PR? |
|
Closing this PR because it has multiple conflicts, hasn’t been updated for over a year and we now have a new PR #6775 |
Proposed changes
This PR pays attention to
keyprop missing at many places where items are rendered under themapfunction. I have added theykeyprop to improve performance and fix the mentioned issue.Issue(s)
#4943
Types of changes
Checklist