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

V3 - FlatList signature, renamed components, performance improvements #77

Merged
merged 13 commits into from
Jan 19, 2019

Conversation

saleel
Copy link
Owner

@saleel saleel commented Jan 19, 2019

Exports two components FlatGrid and SectionGrid
FlatList like signature for FlatGrid
Common styles and calculation
Performance Improvements

@saleel
Copy link
Owner Author

saleel commented Jan 19, 2019

@andersonaddo Please review this PR when you have time. I have refactored SectionGrid as well to use common functions.

Please see readme of v3 branch

if (!staticDimension) {
const { width, height } = e.nativeEvent.layout || {};
const newTotalDimension = horizontal ? height : width;

Copy link
Owner Author

@saleel saleel Jan 19, 2019

Choose a reason for hiding this comment

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

Yes, this is in onLayout. e.nativeEvent.layout will give the available dimension of the whole component. This is executed when the orientation of the device get changed.

Copy link
Contributor

@andersonaddo andersonaddo Jan 19, 2019

Choose a reason for hiding this comment

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

Yeah I just realized that 😄. Wasn't easy to tell when I was reading the diffs. I've now deleted that review.

</View>
);
}
const groupedSections = sections.map(({ title, data }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it probably does, but just making sure. Using .map should keep keys who have their values set to functions intact right? Speaking because of #56 and #57

Copy link
Owner Author

Choose a reason for hiding this comment

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

I did check those two issues. That was a fix for JSON.parse based cloning.
But I didn't see any particular reason we had to clone it. chunkArray do not modify the actual items. Do you remember why you had to clone it?
Map is working just fine. I did put some keys with function as values in the items and it worked fine.

Copy link
Contributor

@andersonaddo andersonaddo Jan 19, 2019

Choose a reason for hiding this comment

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

My original code didn't use .map. It passed the original sections array directly into the chunkArray function, which ended up altering and condensing the sections array with every re-render (I think it had to do with the fact that it was an array of objects, not primitives. But I don't completely remember though)
I didn't realize I could just use map, that's why I cloned the whole array instead.

import React, { Component } from 'react';
import { StyleSheet, View, Text } from 'react-native';
import GridView from 'react-native-super-grid';
import { FlatGrid } from 'react-native-super-grid';

Copy link
Contributor

Choose a reason for hiding this comment

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

This is meant to be an example for the SectionGrid, but it's FlatGrid code that's displayed here.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yea. My bad. Fixing it shortly.

if (!staticDimension) {
const { width, height } = e.nativeEvent.layout || {};
const newTotalDimension = horizontal ? height : width;

Copy link
Owner Author

@saleel saleel Jan 19, 2019

Choose a reason for hiding this comment

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

Yes, this is in onLayout. e.nativeEvent.layout will give the available dimension of the whole component. This is executed when the orientation of the device get changed.

@andersonaddo
Copy link
Contributor

I won't be able to test it out for a few days (laptop issues), but it looks okay from here. You can merge it now if you want, and I'll test it afterwards (especially since you've already tested it before).

@saleel
Copy link
Owner Author

saleel commented Jan 19, 2019

Ok. I will merge it and release to npm. I have tested all scenarios and worked fine for me.

Thanks 👍

@saleel saleel closed this Jan 19, 2019
@saleel saleel reopened this Jan 19, 2019
@saleel saleel merged commit a552f36 into master Jan 19, 2019
@saleel saleel deleted the v3 branch January 19, 2019 21:00
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.

2 participants