-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
Remove defaultProps from FlatList #31798
Conversation
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.
Code analysis results:
eslint
found some issues. Runyarn lint --fix
to automatically fix problems.
Base commit: fa0518d |
Base commit: fa0518d |
@lunaleaps Could you review the pull request? I would be very grateful 😊 |
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.
Looks good, thank you so much for working on this! Small comments about adding more documentation in the props about defaults. Let me know if you want to add those or I can add them on land. @joxw1 Let me know your call and I'll import :)
I can add the documentation, no problem 😊 One thing that came up to my mind while reviewing my changes: Should I introduce a constant for all the |
That's a good idea! I think the concern is that someone introduces a new read of the prop and doesn't set a default value. I think using a constant like that could help make it clearer if they miss it. In other work I've also created a dedicated accessor function like |
Done! For the naming of the accessor functions I followed the VirtualizedList class, where the accessor functions have the variable name as prefix (e.g. @lunaleaps If everything fits now, will you take over the merge? |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…r removeClippedSubviews
…react-native into fix/31602_flat_list_default_props
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@lunaleaps Any updates on this? |
@lunaleaps merged this pull request in 7d5895d. |
Summary
Issue #31602. Remove
defaultProps
fromFlatList
.Changelog
[JavaScript] [Changed] - Remove defaultProps from FlatList
Test Plan
All test for
FlatList
pass.124162098-6dfe4380-da9e-11eb-9fd6-ab47820aebfa.mov