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

Section headers #34

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 2 additions & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
},

"rules": {
"react-native/no-color-literals": "off"
"react-native/no-color-literals": "off",
"react/no-did-update-set-state": "off"
Copy link
Owner

Choose a reason for hiding this comment

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

See comment below for how to avoid this. And just a general tip: it's preferable to type // eslint-disable-next-line react/no-did-update-set-state above your code instead of setting a global rule 👍

},

"globals": {
Expand Down
90 changes: 80 additions & 10 deletions src/ImmutableVirtualizedList/ImmutableVirtualizedList.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Immutable from 'immutable';
import PropTypes from 'prop-types';
import React, { PureComponent } from 'react';
import { Text, VirtualizedList } from 'react-native';
Expand Down Expand Up @@ -26,14 +25,31 @@ class ImmutableVirtualizedList extends PureComponent {
immutableData: (props, propName, componentName) => {
// Note: It's not enough to simply validate PropTypes.instanceOf(Immutable.Iterable),
// because different imports of Immutable.js across files have different class prototypes.
// TODO: Add support for Immutable.Map, etc.
if (Immutable.Map.isMap(props[propName])) {
return new Error(`Invalid prop ${propName} supplied to ${componentName}: Support for Immutable.Map is coming soon. For now, try an Immutable List, Set, or Range.`);
} else if (!utils.isImmutableIterable(props[propName])) {
if (!utils.isImmutableIterable(props[propName])) {
return new Error(`Invalid prop ${propName} supplied to ${componentName}: Must be instance of Immutable.Iterable.`);
}
},

/**
* Whether to treat immutableData as a sectioned list,
* applying section headers.
*/
treatAsSections: PropTypes.bool,
Copy link
Owner

Choose a reason for hiding this comment

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

We can assume they want to render sections if renderSectionHeader is truthy, so this prop can be removed. ListView does the same thing.

Copy link
Author

Choose a reason for hiding this comment

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

Good point!


/**
* A function that returns some {@link PropTypes.element}
* to be rendered as a section header. It will be passed a List
* or Map of the section's items, and the section key.
*/
renderSectionHeader: PropTypes.func,

/**
* A function that returns some {@link PropTypes.element}
* to be rendered as a section row. It will be passed the item,
* and the item's key.
*/
renderRow: PropTypes.func,
Copy link
Owner

Choose a reason for hiding this comment

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

Can we just leave it as renderItem (i.e. remove this prop)?

Copy link
Author

Choose a reason for hiding this comment

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

Yep I think so.


/**
* A plain string, or a function that returns some {@link PropTypes.element}
* to be rendered in place of a `VirtualizedList` when there are no items in the list.
Expand All @@ -58,14 +74,40 @@ class ImmutableVirtualizedList extends PureComponent {

static defaultProps = {
...VirtualizedList.defaultProps,

treatAsSections: false,
renderEmptyInList: 'No data.',
};

state = {
flattenedData: this.props.treatAsSections
? utils.flattenMap(this.props.immutableData)
: null,
}

componentDidUpdate(prevProps) {
const { treatAsSections, immutableData } = this.props;
if (treatAsSections && prevProps.immutableData !== immutableData) {
const flattenedData = utils.flattenMap(immutableData);
this.setState({
flattenedData,
Copy link
Author

Choose a reason for hiding this comment

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

@cooperka I chose to hold flattenedData in state so that it is only created once on each update to immutableData, as the flattening transformation will be fairly expensive.

Copy link
Author

Choose a reason for hiding this comment

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

One alternative, to avoid using state at all, would be to use a WeakMap to cache the results of the flattening transformation. As we are working with immutable data this would be easy to reason about.

Copy link
Owner

Choose a reason for hiding this comment

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

I think using state is fine, but if you put it in willUpdate instead of didUpdate I think it will be more performant (that's why the linter flagged this).

Copy link
Author

Choose a reason for hiding this comment

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

I chose didUpdate because in the react docs for componentWillUpdate() it says "Note that you cannot call this.setState() here; nor should you do anything else (e.g. dispatch a Redux action) that would trigger an update to a React component before componentWillUpdate() returns."

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, componentWillReceiveProps is what you should use here! Not willUpdate.

stickyHeaderIndices: utils.getStickyHeaderIndices(flattenedData),
});
}
}

getVirtualizedList() {
return this.virtualizedListRef;
}

keyExtractor = (item, index) => (
this.props.treatAsSections
? this.state.flattenedData
.keySeq()
.skip(index)
.first()
: String(index)
)

scrollToEnd = (...args) =>
this.virtualizedListRef && this.virtualizedListRef.scrollToEnd(...args);

Expand All @@ -81,6 +123,20 @@ class ImmutableVirtualizedList extends PureComponent {
recordInteraction = (...args) =>
this.virtualizedListRef && this.virtualizedListRef.recordInteraction(...args);

renderItem = (info) => (
this.props.treatAsSections
? utils.isSectionHeader(info.item)
? this.renderSectionHeader(
info.item,
this.keyExtractor(info.item, info.index),
)
: this.renderRow(
info.item,
this.keyExtractor(info.item, info.index),
)
: this.props.renderItem(info)
)

renderEmpty() {
const {
immutableData, renderEmpty, renderEmptyInList, contentContainerStyle,
Expand All @@ -107,15 +163,29 @@ class ImmutableVirtualizedList extends PureComponent {
}

render() {
const { immutableData, renderEmpty, renderEmptyInList, ...passThroughProps } = this.props;
const {
immutableData,
treatAsSections,
renderEmpty,
renderEmptyInList,
...passThroughProps
} = this.props;

const { flattenedData, stickyHeaderIndices } = this.state;

return this.renderEmpty() || (
<VirtualizedList
ref={(component) => { this.virtualizedListRef = component; }}
data={immutableData}
getItem={(items, index) => utils.getValueFromKey(index, items)}
data={treatAsSections ? flattenedData : immutableData}
getItem={(items, index) => (
treatAsSections
? items.skip(index).first()
: utils.getValueFromKey(index, items)
)}
getItemCount={(items) => (items.size || 0)}
keyExtractor={(item, index) => String(index)}
keyExtractor={this.keyExtractor}
stickyHeaderIndices={treatAsSections ? stickyHeaderIndices : null}
renderItem={this.renderItem}
{...passThroughProps}
/>
);
Expand Down
22 changes: 22 additions & 0 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,28 @@ const utils = {
return immutableData.every((item) => !item || item.isEmpty());
},

flattenMap(data) {
return data.reduce(
(flattened, section, key) => flattened.set(key, section).merge(section),
Immutable.OrderedMap().asMutable(),
Copy link
Owner

Choose a reason for hiding this comment

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

I think I understand but please clarify -- is asMutable being used here for performance reasons, so we don't have to return a new Immutable object during each reduction step?

Copy link
Author

Choose a reason for hiding this comment

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

Yes exactly, it's a little trick I learned after months of using immutablejs. Within our flattenMap function, it's safe to make the collection mutable while we build it as long as we call asImmutable() on it before we return it.

).asImmutable();
},

isSectionHeader(item) {
return Immutable.Map.isMap(item) || Immutable.List.isList(item);
Copy link
Owner

Choose a reason for hiding this comment

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

Why are Map and List necessarily section headers? I don't think I understand how this method works.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, I wasn't totally happy about this. In the flattenMap method, I'm setting the value of the section header items to be their respective section, in other words:

Map {
  section1: Map {
    key1: val1,
    key2: val2
  },
  section2: Map {
    key3: val3,
    key4: val4
  }
}

// gets flattened to:

Map {
  section1: Map {
    key1: val1,
    key2: val2
  },
  key1: val1,
  key2: val2,
  section2: Map {
    key3: val3,
    key4: val4
  },
  key3: val3,
  key4: val4
}

One way, therefore, of identifying whether or not an item is a section header, is by querying if the item's value is a Map or a List. Do you think there's a better way?

Copy link
Owner

Choose a reason for hiding this comment

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

Do you happen to have a link to where the original ListView does something like this?

Copy link
Author

Choose a reason for hiding this comment

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

VirtualizedSectionList does it like this: https://github.com/facebook/react-native/blob/master/Libraries/Lists/VirtualizedSectionList.js#L175-L224

For each index, it runs through the entire collection to work out if the index refers to a section header. I think this could be expensive, especially given that we are working with a Map which has keys rather than indexes so we'd have to convert to an indexed collection too.

Actually, I think I can use stickyHeaderIndices, as we have already computed these. Will put this in the next commit!

},

getStickyHeaderIndices(items) {
return items
.valueSeq()
.reduce((arr, item, i) => {
if (utils.isSectionHeader(item)) {
arr.push(i);
}
return arr;
}, []);
},

};

export default utils;