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

Implement FlatList everywhere #536

Merged
merged 35 commits into from
Sep 25, 2020
Merged

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Sep 24, 2020

@marcaaron marcaaron self-assigned this Sep 24, 2020
@Julesssss
Copy link
Contributor

Tested the Desktop build with @muttmuure -- what an improvement! Mobile performance is also good.

For web I'm not being scrolled to the newest comment, but I'll review this again once the PR is complete.


there are some flashes of missing content while scrolling

From the FlatList docs, this seems to be a known trade-off

In order to constrain memory and enable smooth scrolling, content is rendered asynchronously offscreen. This means it's possible to scroll faster than the fill rate and momentarily see blank content. This is a tradeoff that can be adjusted to suit the needs of each application, and we are working on improving it behind the scenes.

Perhaps getItemLayout might be of help here:

Adding getItemLayout can be a great performance boost for lists of several hundred items

we would need to calculate height based on row data. This React snippet seems to achieve this with object.nativeEvent.layout.height

@marcaaron
Copy link
Contributor Author

Thanks @Julesssss that snack snippet looks really cool. I tried it out on web, but the performance was noticeably worse than react-window. On mobile it led to some reports showing up without comments. Probably going to keep going on the path that I'm on, but wouldn't be opposed to looking into that again in the future.

tgolen
tgolen previously requested changes Sep 24, 2020
src/components/InvertedFlatList/index.js Outdated Show resolved Hide resolved
src/page/home/MainView.js Outdated Show resolved Hide resolved
src/page/home/report/ReportActionItem.js Outdated Show resolved Hide resolved
src/page/home/report/ReportActionItem.js Outdated Show resolved Hide resolved
src/page/home/report/ReportActionItem.js Outdated Show resolved Hide resolved
// This is the created action and the very first action so it cannot be a consecutive comment.
if (actionIndex === 0) {
return false;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not relevant anymore since I've added logic to remove all non comments before we get to this point

@marcaaron marcaaron changed the title Use react-window to render React Native components when on web/desktop. Implement FlatList in mobile. Implement FlatList everywhere Sep 25, 2020
Julesssss
Julesssss previously approved these changes Sep 25, 2020
Co-authored-by: Andrew Gable <andrew@expensify.com>
@AndrewGable AndrewGable dismissed tgolen’s stale review September 25, 2020 23:20

Changes made to PR

@AndrewGable AndrewGable merged commit f94f424 into master Sep 25, 2020
@AndrewGable AndrewGable deleted the marcaaron-noInvertFlatlist branch September 25, 2020 23:21
if (_.size(prevProps.reportActions) !== _.size(this.props.reportActions)) {
// If a new comment is added and it's from the current user scroll to the bottom otherwise
// leave the user positioned where they are now in the list.
if (lastItem(this.props.reportActions).actorEmail === this.props.session.email) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like a few warnings got added here, can we prevent these?

  • 'session' is missing in props validation
  • 'session.email' is missing in props validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I'll do a follow up to take care of these now.

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.

5 participants