Section headers #34

benadamstyles · 2018-01-23T13:16:37Z

This is just my initial commit – it would be really helpful if @cooperka you could take a glance at this before I continue. I have had to make some possibly controversial decisions, most notably introducing a setState in componentDidUpdate – for which I had to make a change in .eslintrc.

I welcome any comments, now matter how challenging they are!

benadamstyles · 2018-01-23T13:18:44Z

src/ImmutableVirtualizedList/ImmutableVirtualizedList.js

+    if (treatAsSections && prevProps.immutableData !== immutableData) {
+      const flattenedData = utils.flattenMap(immutableData);
+      this.setState({
+        flattenedData,


@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.

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.

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).

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."

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

cooperka

This is looking great @Leeds-eBooks! Thank you again for the help. I left several comments for whenever you find time to look it over.

cooperka · 2018-01-29T22:30:41Z

src/ImmutableVirtualizedList/ImmutableVirtualizedList.js

+    if (treatAsSections && prevProps.immutableData !== immutableData) {
+      const flattenedData = utils.flattenMap(immutableData);
+      this.setState({
+        flattenedData,


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).

cooperka · 2018-01-29T22:32:37Z

.eslintrc

@@ -8,7 +8,8 @@
  },

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


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 👍

cooperka · 2018-01-29T22:33:32Z

src/ImmutableVirtualizedList/ImmutableVirtualizedList.js

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


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

Good point!

cooperka · 2018-01-29T22:36:07Z

src/ImmutableVirtualizedList/ImmutableVirtualizedList.js

+     * to be rendered as a section row. It will be passed the item,
+     * and the item's key.
+     */
+    renderRow: PropTypes.func,


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

Yep I think so.

cooperka · 2018-01-29T22:39:01Z

src/utils.js

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


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?

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.

cooperka · 2018-01-29T22:43:07Z

src/utils.js

+  },
+
+  isSectionHeader(item) {
+    return Immutable.Map.isMap(item) || Immutable.List.isList(item);


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

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?

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

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!

removed isSectionHeader() in favour of querying stickyHeaderIndices

cooperka · 2018-02-09T16:32:45Z

I made a few minor style tweaks.

I think I might be passing in the wrong data format because I get TypeError: Expected [K, V] tuple at utils.flattenMap at reduce (this happens on both your original branch and after my changes). Any thoughts? Here's my example app diff to try it out:

diff --git a/example/index.js b/example/index.js
index 3fd0fe8..e5abcac 100644
--- a/example/index.js
+++ b/example/index.js
@@ -1,7 +1,7 @@
 import { AppRegistry } from 'react-native';
 
 // Choose one:
-import App from './src/ImmutableListViewExample';
-// import App from './src/ImmutableVirtualizedListExample';
+// import App from './src/ImmutableListViewExample';
+import App from './src/ImmutableVirtualizedListExample';
 
 AppRegistry.registerComponent('ImmutableListViewExample', () => App);
diff --git a/example/src/ImmutableVirtualizedListExample.js b/example/src/ImmutableVirtualizedListExample.js
index 5465c12..e5996c7 100644
--- a/example/src/ImmutableVirtualizedListExample.js
+++ b/example/src/ImmutableVirtualizedListExample.js
@@ -20,18 +20,23 @@ import utils from './utils';
  *
  */
 function ImmutableVirtualizedListExample() {
+  const sectionData = Immutable.fromJS({
+    sec1: [1, 2, 3],
+  });
+
   return (
     <GenericListExample
       ListComponent={ImmutableVirtualizedList}
       listComponentProps={{
         renderItem: utils.renderItem,
+        renderSectionHeader: utils.renderSectionHeader,
         keyExtractor: utils.trivialKeyExtractor,
       }}
 
-      initialDataA={Immutable.List(['Simple', 'List', 'of', 'Items'])}
+      initialDataA={sectionData}
       dataMutatorA={(data) => data.set(3, 'This value was changed!')}
 
-      initialDataB={Immutable.Range(1, 100)}
+      initialDataB={sectionData}
       dataMutatorB={(data) => data.toSeq().map((n) => n * 2)}
     />
   );

benadamstyles · 2018-02-09T16:34:50Z

Thanks for making these tweaks. I've been unexpectedly busy this week which is why I haven't had a chance to continue working on this, but it's really high on my to-do list! At the latest, I have 17th Feb free to work on this. Got to add tests, mainly.

benadamstyles · 2018-02-09T16:37:22Z

Oh, just seen your question! Will take a look at this asap. It's not obvious to me off the top of my head.

cooperka · 2018-02-09T16:38:24Z

It's all good, I know how life can be. Tests would be great, and will probably resolve my issue! Either by helping me understand the data format or by catching a bug ;) Take your time!

benadamstyles · 2018-02-09T17:34:57Z

Ok it's because my usage of merge in flattenMap only handles a Map with Map rows, rather than List rows. Will work on this next!

benadamstyles · 2018-02-10T11:10:30Z

Ok think this is working now!

benadamstyles · 2018-02-17T20:32:08Z

Just catching up with this – does anything remain to be done? Does it need more tests do you think?

cooperka · 2018-02-18T15:58:59Z

Hmm, it still doesn't seem to work with the example app using the code diff I pasted above. The list is simply blank (though no errors anymore). It also gets into a strange state when loading and erroring (see screenshot). Do you see the same thing as me?

benadamstyles · 2018-02-18T16:57:54Z

@cooperka Ah yes, I never got round to testing with a live app – just doing that now. Is there a nicer way to make changes in the package and see it updated in the example app than killing the app and running yarn again?

cooperka · 2018-02-18T17:14:26Z

In an earlier version of RN + yarn you could simply npm install file:.. which took about 5 seconds and it would live update. This no longer works unfortunately.

It might work to yarn add file:.., restart the packager, and simply refresh the app (ctrl+R on ios or R+R on android).

cooperka · 2018-02-18T17:15:40Z

I know there are other automated solutions; if you're familiar with one feel free to recommend it. I've never needed something like that until now since the npm trick used to work.

benadamstyles · 2018-02-18T17:53:09Z

src/ImmutableVirtualizedList/ImmutableVirtualizedList.js

+        ? renderSectionHeader
+        : renderItem;
+
+      return renderMethod(info.item, this.keyExtractor(info.item, info.index));


Ok the problem stems from this line. In your example app, you’re (understandably!) assuming that renderItem() will be called with the whole info object (as it is if we are dealing with a flat list without section headers, 3 lines below this comment). Will push a fix asap!

…` as we do for flat lists

cooperka · 2018-02-18T18:05:39Z

The items display now, but it shows 123 as the section header instead of sec1 as expected. There are also still 4 "Loading" rows. The code in GenericListExample might need to be updated to fix this; I haven't diagnosed any further than just visually.

benadamstyles · 2018-02-18T18:09:45Z

Yeah sorry, got to change something in the example app's code. Coming right up 🙂

benadamstyles · 2018-02-18T19:33:17Z

Ah I think I remember now why I didn't originally pass the whole info object to renderSectionHeader(): I think I took my cue from the test-utils renderers. In the row renderer, you expect an info object with a item property, but in the sectionHeader renderer, you expect the sectionData directly (i.e. not wrapped in an info object. Should I revert my last commit, or refactor all code throughout the test utils and example app to read the sectionData as being at info.item (as it is for row data)?

cooperka · 2018-02-18T20:23:58Z

My intuition is that we should use the same data format conventions as the original ListView, but the function arguments should be formatted like VirtualizedList for consistency.

Eventually I'd love to release ImmutableSectionList as a counterpart to SectionList, but that would be more work since it's a very different API with extra features.

My recommendation for this PR (feel free to suggest changes):

// This data:
Immutable.fromJS({
  sec1: [1, 2, 3],
})

// Or, equivalently:
Immutable.fromJS({
  sec1: { r1: 1, r2: 2, r3: 3 },
})

Would render as sec1 section with 3 items: 1, 2, 3.

The methods would be called as follows:

renderSectionHeader({ sectionData, sectionID })
renderItem({ item, index })

So test-utils.js will need to be updated to include a renderVirtualizedSectionHeader method with these arguments (the existing renderSectionHeader is for ImmutableListView only).

What do you think?

taranda · 2019-04-11T04:25:15Z

@cooperka and @benadamstyles: Have you made any progress on this? I would love to see section headers implemented, and I like the shape of the input data.

Do you need any help pushing this over the goal line?

alexiri · 2019-06-22T09:00:43Z

We could really use this functionality as well. We're currently using ImmutableListView, but we're seeing some performance issues.

initial commit

f5d14ca

benadamstyles mentioned this pull request Jan 23, 2018

Support for section headers (SectionList?) #25

Open

benadamstyles commented Jan 23, 2018

View reviewed changes

lint fixes

d83fa3c

cooperka reviewed Jan 29, 2018

View reviewed changes

benadamstyles and others added 2 commits January 31, 2018 18:47

improvements based on comments from @cooperka

cba8549

removed isSectionHeader() in favour of querying stickyHeaderIndices

Code style tweaks

39e81fd

handle Map of Lists, add tests

073c262

cooperka force-pushed the master branch 6 times, most recently from 255bb12 to efdc474 Compare February 11, 2018 23:14

benadamstyles commented Feb 18, 2018

View reviewed changes

pass whole info object to renderItem() and `renderSectionHeader()…

6394033

…` as we do for flat lists

benadamstyles added 2 commits February 18, 2018 19:49

fix renderSectionHeader branch condition

53ac8fb

update sectionHeader test snapshots

090b2d4

Section headers #34

Are you sure you want to change the base?

Section headers #34

Conversation

benadamstyles commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cooperka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cooperka commented Feb 9, 2018 • edited Loading

benadamstyles commented Feb 9, 2018

benadamstyles commented Feb 9, 2018

cooperka commented Feb 9, 2018

benadamstyles commented Feb 9, 2018

benadamstyles commented Feb 10, 2018

benadamstyles commented Feb 17, 2018

cooperka commented Feb 18, 2018

benadamstyles commented Feb 18, 2018

cooperka commented Feb 18, 2018 • edited Loading

cooperka commented Feb 18, 2018

Choose a reason for hiding this comment

cooperka commented Feb 18, 2018

benadamstyles commented Feb 18, 2018

benadamstyles commented Feb 18, 2018

cooperka commented Feb 18, 2018 • edited Loading

taranda commented Apr 11, 2019

alexiri commented Jun 22, 2019

cooperka commented Feb 9, 2018 •

edited

Loading

cooperka commented Feb 18, 2018 •

edited

Loading

cooperka commented Feb 18, 2018 •

edited

Loading