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

Add onStartReached and sync onEndReached doc #3721

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

janicduplessis
Copy link
Contributor

  • Add docs for onStartReached for FlatList and SectionList
  • Sync up docs with source code for onEndReached for FlatList and SectionList

@netlify
Copy link

netlify bot commented May 15, 2023

Deploy Preview for react-native ready!

Name Link
🔨 Latest commit 29db1b5
🔍 Latest deploy log https://app.netlify.com/sites/react-native/deploys/6464ebc0ce221e00089efaa8
😎 Deploy Preview https://deploy-preview-3721--react-native.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@Simek Simek left a comment

Choose a reason for hiding this comment

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

Refs:

Hey @janicduplessis, thank you for the followup on core PR! 👍

After looking at the code, I think it would be nice to clarify where exactly those propos are defined. By that I mean that the props should not be a part of FlatList and SectionList, but be listed in VirtualizedList props, which are than extended but listed earlier components. We already show the parent props link at the top of API documentation.

There were some concerns and benefits around duplicating props of parent in child components, it's fine to leave it as is for now since is a broader topic, but no matter which approach we choose, the VirtualizedList props also need to be updated:

@janicduplessis
Copy link
Contributor Author

@Simek Sounds good, I didn’t see that shared doc! Will move onStart/End there.

@janicduplessis
Copy link
Contributor Author

@Simek Done!

Copy link
Collaborator

@Simek Simek left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for the updates! 👍

We can ship this since changes are already a part of upcoming 0.72 release, but let's wait for one more review, to make sure we are aligned on the approach.

@janicduplessis
Copy link
Contributor Author

Ping @NickGerleman @cortinico

@Simek
Copy link
Collaborator

Simek commented Jun 13, 2023

I have asked around, and it seems that people do not have a strong preference around that, let's ship this as is. We can always follow up with changes, if someone will find this pattern confusing/cumbersome. 🙂

@Simek Simek merged commit 4e7dbc0 into facebook:main Jun 13, 2023
@janicduplessis janicduplessis deleted the patch-1 branch June 13, 2023 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants