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.

Sign up for GitHub

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

Jump to bottom

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

Closed
wants to merge 15 commits into from
Closed

[VirtualizedSectionList] - Externalise and handle any sort of data blob #20787

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
90baadb
Allow VirtualizedSectionList to handle any sort of data blob
magrinj Aug 22, 2018
cc92082
Externalize VirtualizedSectionList
magrinj Aug 22, 2018
66c365a
computeState use this, so keep using UNSAFE_componentWillReceiveProps
magrinj Aug 22, 2018
c00db10
Fix flow error getItem function
magrinj Aug 22, 2018
0762ef6
Fix some errors
magrinj Aug 27, 2018
b1fe9b9
Fix lint error & refreshing flow error
magrinj Aug 27, 2018
880c4dd
Small fix
magrinj Aug 28, 2018
0168f7a
Update VirtualizedSectionList-test.js
hramos Jan 15, 2019
a25a136
Fix conflicts
magrinj Jan 16, 2019
dfe4994
Remove shadow variable
magrinj Jan 16, 2019
5437f8b
Remove unused variable
magrinj Jan 16, 2019
edebcef
Remove getItemParam and replace by getItem and fix flow
magrinj Feb 27, 2019
4ff6035
Run prettier
magrinj Feb 27, 2019
32a5e4c
Merge remote-tracking branch 'upstream/master' into virtualized-secti…
magrinj Apr 18, 2019
1f00f8e
Merge branch 'master' into virtualized-section-list
magrinj Apr 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Libraries/Lists/MetroListView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ type NormalProps = {
/**
* Set this true while waiting for new data from a refresh.
*/
refreshing?: boolean,
refreshing?: ?boolean,
/**
* If true, renders items next to each other horizontally instead of stacked vertically.
*/
Expand Down Expand Up @@ -112,7 +112,7 @@ class MetroListView extends React.Component<Props, $FlowFixMeState> {
* for React. To see the error delete this comment and run Flow.
*/
<RefreshControl
refreshing={props.refreshing}
refreshing={props.refreshing || false}
onRefresh={props.onRefresh}
/>
}
Expand Down
29 changes: 28 additions & 1 deletion Libraries/Lists/SectionList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,14 +322,41 @@ class SectionList<SectionT: SectionBase<any>> extends React.PureComponent<
}
}

constructor(props: Props<SectionT>) {
super(props);
this._checkProps(this.props);
}

componentDidUpdate(prevProps: Props<SectionT>) {
this._checkProps(this.props);
}

_checkProps(props: Props<SectionT>) {
const {getItem, getItemCount, getItemParam} = props;
cpojer marked this conversation as resolved.
Show resolved Hide resolved

if (__DEV__) {
if (getItem || getItemCount || getItemParam) {
console.warn('SectionList does not support custom data formats.');
}
}
}

render() {
const List = this.props.legacyImplementation
? MetroListView
: VirtualizedSectionList;
/* $FlowFixMe(>=0.66.0 site=react_native_fb) This comment suppresses an
* error found when Flow v0.66 was deployed. To see the error delete this
* comment and run Flow. */
return <List {...this.props} ref={this._captureRef} />;
return (
<List
{...this.props}
ref={this._captureRef}
getItemCount={items => items.length}
getItem={(items, index) => items[index]}
getItemParam={(item, param) => item[param]}
/>
);
}

_wrapperListRef: MetroListView | VirtualizedSectionList<any>;
Expand Down
126 changes: 68 additions & 58 deletions Libraries/Lists/VirtualizedSectionList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,34 +20,16 @@ import type {Props as VirtualizedListProps} from 'VirtualizedList';

type Item = any;
type SectionItem = any;
type Option = any;

type SectionBase = {
// Must be provided directly on each section.
data: $ReadOnlyArray<SectionItem>,
key?: string,

// Optional props will override list-wide props just for this section.
renderItem?: ?({
item: SectionItem,
index: number,
section: SectionBase,
separators: {
highlight: () => void,
unhighlight: () => void,
updateProps: (select: 'leading' | 'trailing', newProps: Object) => void,
},
}) => ?React.Element<any>,
ItemSeparatorComponent?: ?React.ComponentType<any>,
keyExtractor?: (item: SectionItem, index: ?number) => string,

// TODO: support more optional/override props
// FooterComponent?: ?ReactClass<any>,
// HeaderComponent?: ?ReactClass<any>,
// onViewableItemsChanged?: ({viewableItems: Array<ViewToken>, changed: Array<ViewToken>}) => void,
};
type SectionBase = Object;
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 a regression. Could you work on putting back the proper Flow Type and running flow on the react-native repo to verify it?

Copy link
Contributor Author

@magrinj magrinj Feb 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The SectionBase type can't be specified, because it can handle different type of datas, like classic js object or Immutable object, by exemple the data type of VirtualizedList is any.
For SectionList component SectionBase can be specified because we know the data must be javascript object.


type RequiredProps<SectionT: SectionBase> = {
sections: $ReadOnlyArray<SectionT>,
sections: $ReadOnlyArray<SectionT> | $ReadOnly<SectionT>,
/**
* A generic accessor for extracting a section option from any sort of data blob.
*/
getItemParam: (section: any, name: string) => ?Option,
};

type OptionalProps<SectionT: SectionBase> = {
Expand Down Expand Up @@ -115,7 +97,8 @@ type OptionalProps<SectionT: SectionBase> = {
*/
refreshing?: ?boolean,
};

/* $FlowFixMe - this Props seems to be missing a bunch of stuff. Remove this
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we fix this instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not really an expert with complex flow typing, I can try to remove this, but I'm not sure if I'm going to fix something of fix it in a good way :/

* comment to see the errors */
export type Props<SectionT> = RequiredProps<SectionT> &
OptionalProps<SectionT> &
VirtualizedListProps;
Expand Down Expand Up @@ -147,7 +130,10 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
}) {
let index = params.itemIndex + 1;
for (let ii = 0; ii < params.sectionIndex; ii++) {
index += this.props.sections[ii].data.length + 2;
const section = this.props.getItem(this.props.sections, ii);
const sectionData = this.props.getItemParam(section, 'data');
const dataLength = this.props.getItemCount(sectionData);
index += dataLength + 2;
}
const toIndexParams = {
...params,
Expand All @@ -172,10 +158,14 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
_computeState(props: Props<SectionT>): State {
const offset = props.ListHeaderComponent ? 1 : 0;
const stickyHeaderIndices = [];
const itemCount = props.sections.reduce((v, section) => {
stickyHeaderIndices.push(v + offset);
return v + section.data.length + 2; // Add two for the section header and footer.
}, 0);
const itemCount = props.sections
? props.sections.reduce((v, section) => {
const sectionData = props.getItemParam(section, 'data');

stickyHeaderIndices.push(v + offset);
return v + props.getItemCount(sectionData) + 2; // Add two for the section header and footer.
}, 0)
: 0;

return {
childProps: {
Expand All @@ -184,7 +174,7 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
ItemSeparatorComponent: undefined, // Rendered with renderItem
data: props.sections,
getItemCount: () => itemCount,
getItem,
getItem: (sections, index) => getItem(props, sections, index),
keyExtractor: this._keyExtractor,
onViewableItemsChanged: props.onViewableItemsChanged
? this._onViewableItemsChanged
Expand Down Expand Up @@ -221,38 +211,45 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
} {
let itemIndex = index;
const defaultKeyExtractor = this.props.keyExtractor;
for (let ii = 0; ii < this.props.sections.length; ii++) {
const section = this.props.sections[ii];
const key = section.key || String(ii);
for (let ii = 0; ii < this.props.getItemCount(this.props.sections); ii++) {
const section = this.props.getItem(this.props.sections, ii);
const sectionData = this.props.getItemParam(section, 'data');
const key = this.props.getItemParam(section, 'key') || String(ii);
itemIndex -= 1; // The section adds an item for the header
if (itemIndex >= section.data.length + 1) {
itemIndex -= section.data.length + 1; // The section adds an item for the footer.
if (itemIndex >= this.props.getItemCount(sectionData) + 1) {
itemIndex -= this.props.getItemCount(sectionData) + 1; // The section adds an item for the footer.
} else if (itemIndex === -1) {
return {
section,
key: key + ':header',
index: null,
header: true,
trailingSection: this.props.sections[ii + 1],
trailingSection: this.props.getItem(this.props.sections, ii + 1),
};
} else if (itemIndex === section.data.length) {
} else if (itemIndex === this.props.getItemCount(sectionData)) {
return {
section,
key: key + ':footer',
index: null,
header: false,
trailingSection: this.props.sections[ii + 1],
trailingSection: this.props.getItem(this.props.sections, ii + 1),
magrinj marked this conversation as resolved.
Show resolved Hide resolved
};
} else {
magrinj marked this conversation as resolved.
Show resolved Hide resolved
const keyExtractor = section.keyExtractor || defaultKeyExtractor;
const keyExtractor =
this.props.getItemParam(section, 'keyExtractor') ||
defaultKeyExtractor;
const sectionData = this.props.getItemParam(section, 'data');
return {
section,
key: key + ':' + keyExtractor(section.data[itemIndex], itemIndex),
key:
key +
':' +
keyExtractor(this.props.getItem(sectionData, itemIndex), itemIndex),
index: itemIndex,
leadingItem: section.data[itemIndex - 1],
leadingSection: this.props.sections[ii - 1],
trailingItem: section.data[itemIndex + 1],
trailingSection: this.props.sections[ii + 1],
leadingItem: this.props.getItem(sectionData, itemIndex - 1),
leadingSection: this.props.getItem(this.props.sections, ii - 1),
trailingItem: this.props.getItem(sectionData, itemIndex + 1),
trailingSection: this.props.getItem(this.props.sections, ii + 1),
};
}
}
Expand All @@ -264,7 +261,9 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
if (!info) {
return null;
}
const keyExtractor = info.section.keyExtractor || this.props.keyExtractor;
const keyExtractor =
this.props.getItemParam(info.section, 'keyExtractor') ||
this.props.keyExtractor;
return {
...viewable,
index: info.index,
magrinj marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -309,7 +308,9 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
return renderSectionFooter ? renderSectionFooter({section}) : null;
}
} else {
const renderItem = info.section.renderItem || this.props.renderItem;
const renderItem =
this.props.getItemParam(info.section, 'renderItem') ||
this.props.renderItem;
const SeparatorComponent = this._getSeparatorComponent(index, info);
invariant(renderItem, 'no renderItem!');
return (
Expand Down Expand Up @@ -351,10 +352,13 @@ class VirtualizedSectionList<SectionT: SectionBase> extends React.PureComponent<
return null;
}
const ItemSeparatorComponent =
info.section.ItemSeparatorComponent || this.props.ItemSeparatorComponent;
this.props.getItemParam(info.section, 'ItemSeparatorComponent') ||
this.props.ItemSeparatorComponent;
const {SectionSeparatorComponent} = this.props;
const isLastItemInList = index === this.state.childProps.getItemCount() - 1;
const isLastItemInSection = info.index === info.section.data.length - 1;
const sectionData = this.props.getItemParam(info.section, 'data');
const isLastItemInSection =
info.index === this.props.getItemCount(sectionData) - 1;
if (SectionSeparatorComponent && isLastItemInSection) {
return SectionSeparatorComponent;
}
Expand Down Expand Up @@ -441,7 +445,7 @@ class ItemWithSeparator extends React.Component<
},
updateProps: (select: 'leading' | 'trailing', newProps: Object) => {
const {LeadingSeparatorComponent, cellKey, prevCellKey} = this.props;
if (select === 'leading' && LeadingSeparatorComponent != null) {
if (select === 'leading' && LeadingSeparatorComponent !== null) {
this.setState(state => ({
leadingSeparatorProps: {...state.leadingSeparatorProps, ...newProps},
}));
Expand Down Expand Up @@ -516,22 +520,28 @@ class ItemWithSeparator extends React.Component<
}
}

function getItem(sections: ?$ReadOnlyArray<Item>, index: number): ?Item {
function getItem(
props: Props<SectionBase>,
sections: ?$ReadOnlyArray<Item>,
index: number,
): ?Item {
if (!sections) {
return null;
}
let itemIdx = index - 1;
for (let ii = 0; ii < sections.length; ii++) {
if (itemIdx === -1 || itemIdx === sections[ii].data.length) {
for (let ii = 0; ii < props.getItemCount(sections); ii++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Working on this at FB. I think this is supposed to stay sections.length here because sections is always an array. I'm changing this internally, no need to do anything, just wanted to put this comment here for your information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpojer Thanks ! In fact it can be something else, by exemple if you use an immutable list, to get his length the method is size and not length so it's better to keep it outside to handle immutable data

const section = props.getItem(props.sections, ii);
const sectionData = props.getItemParam(section, 'data');
if (itemIdx === -1 || itemIdx === props.getItemCount(sectionData)) {
// We intend for there to be overflow by one on both ends of the list.
// This will be for headers and footers. When returning a header or footer
// item the section itself is the item.
return sections[ii];
} else if (itemIdx < sections[ii].data.length) {
return section;
} else if (itemIdx < props.getItemCount(sectionData)) {
// If we are in the bounds of the list's data then return the item.
return sections[ii].data[itemIdx];
return props.getItem(sectionData, itemIdx);
} else {
itemIdx -= sections[ii].data.length + 2; // Add two for the header and footer
itemIdx -= props.getItemCount(sectionData) + 2; // Add two for the header and footer
}
}
return null;
Expand Down
Loading