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

added onload event #180

Merged
merged 7 commits into from
Mar 14, 2022
Merged

added onload event #180

merged 7 commits into from
Mar 14, 2022

Conversation

naqvitalha
Copy link
Collaborator

Description

Added onLoadEvent to report once items have been rendered.

Checklist

@@ -338,6 +354,7 @@ class FlashList<T> extends React.PureComponent<
finalRenderAheadOffset={finalDrawDistance}
renderAheadStep={finalDrawDistance}
initialRenderIndex={initialScrollIndex || undefined}
onItemLayout={this.raiseOnLoadEvent}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't 100% follow why raiseOnLoadEvent can be called either from componentDidMount or from ProgressiveListView's onItemLayout - could you please explain it again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's say if I pass empty data and expect list to render ListEmptyComponent then I check that in did mount and raise the event.
For other cases I listen to onItemLayout of RLV which is raised whenever any item's onLayout is triggered which is a great proxy as this event is raised after an item is rendered.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does it mean that if list is not empty, onLayout of any item gets called before componentDidMount?

Copy link
Collaborator Author

@naqvitalha naqvitalha Mar 14, 2022

Choose a reason for hiding this comment

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

It's the other way actually. For FlashList onLayout will come after componentDidMount. There won't be any rendered items when the list mounts.
The check on mount method is only needed if list is empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

For FlashList onLayout will come after componentDidMount.

Doesn't it mean that in both cases (empty vs non-empty list) componentDidMount is called first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes that's true. But for empty list there won't be an onLayout (as there are no items) so I need to check in componentDidMount. I can check if data is empty I can simply raise the event there and then.

Copy link
Contributor

@fortmarek fortmarek Mar 14, 2022

Choose a reason for hiding this comment

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

Doesn't it mean that in both cases (empty vs non-empty list) componentDidMount is called first?

I think so, but then this will be false and the event won't be raised.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fortmarek that's the right thing to do if you think about it. If someone renders empty list component the load of the component should be considered as the load event. Passing data in the next cycle is an update and you can't know how much time app spent in fetching the data.
As soon as list finishes what's expected out of the first render we should raise onLoadComplete.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the right thing to do if you think about it. If someone renders empty list component the load of the component should be considered as the load event. Passing data in the next cycle is an update and you can't know how much time app spent in fetching the data.

I agree, I was being in support of the current solution 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I missed the point I think :P

src/FlashList.tsx Outdated Show resolved Hide resolved
src/FlashList.tsx Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/FlashList.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@fortmarek fortmarek left a comment

Choose a reason for hiding this comment

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

Apart from the web documentation, my comments are nits, so I'm gonna approve this 👍

@naqvitalha naqvitalha merged commit a73a74b into main Mar 14, 2022
@naqvitalha naqvitalha deleted the onLoadEvent branch March 14, 2022 18:15
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.

4 participants