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

Feedback on RecyclerListView #35

Closed
necolas opened this issue Aug 27, 2017 · 16 comments
Closed

Feedback on RecyclerListView #35

necolas opened this issue Aug 27, 2017 · 16 comments

Comments

@necolas
Copy link

necolas commented Aug 27, 2017

Hi, following up on some conversations with @naqvitalha over in the react-native-web repo. Here are some findings/comparisons to our scroller from @madnl, who tried the component in Twitter Lite's timelines (on desktop Chrome):

  • Proximity events
    • only support for 'bottom reached' event (no distinction between near and at)
    • no at top, near top events
  • new items injected at top leads to loss of scroll position
  • after items injected at top - scrolling upwards leads to constant scroll jumps (even in Desktop Chrome, which is the most forgiving of browsers for scroll adjustments)
  • strange layout issues where items appeared offsetted to the left

  • unsure how virtualization is happening - all items seem to be rendered at once (coming back to a timeline has noticeable lag)

  • frequent stack overflows

  • A mechanism for saving position when coming back to timeline exists, but not sure how it is applied or if it works (see ContextProvider).

  • Sometimes items overlap

Although the following are not blocking issues since our current scroller does not have them either, it should be stated that this library does not seem to provide support for:

  • reverse-order lists
  • tombstones or entry previews to support fast scrolling without the rendering overhead.

For the most part, it could work, although there is a point where it might not be generally usable for our timelines - given that timelines change in a non-linear way, there does not seem to be an anchoring mechanism that ensures the most visible item stays put when new items are rendered or removed above it. This would require identifying the items by key, instead of by index how it is now.

@naqvitalha
Copy link
Collaborator

@madnl @necolas thanks for the feedback. Find my responses inline:

Proximity events
onEndReachedThreshold lets you specify how much in advance you get the callback. However, additional events should be trivial to add since onScroll is exposed.

new items injected at top leads to loss of scroll position
Yeah that might be the case, never designed for this scenario. I think anchoring logic can be built easily for the web. On react native it's a much bigger challenge due to it's async nature. I'll look into this. This will also make reverse order lists possible.

strange layout issues where items appeared offsetted to the left
Due to staggered grid implementation. If you have set flex:1 on your content it'll be fine. In non deterministic scenario the ListView relayouts using exact rendered dimensions. ListView, itself, will not stretch the content.

Sometimes items overlap
Are you guys using this with React Native Web? I've seen this happening due to missing onLayout callbacks in some cases.

A mechanism for saving position when coming back to timeline exists, but not sure how it is applied or if it works
Yes context provider takes care of that and enables quick redraw. I'll put forth a sample soon.

Virtualization
All the other issues like stack overflow etc seem to be happening due to an incorrect layout provider. If you can share that I can take a look. RecyclerListView works in a lot of way like UICollectionView. It uses estimated or exact heights to figure out how much items will fit the screen. Is it possible that you have given very small estimated dimensions or, you momentarily render items with 0 height? 0 height will cause RecyclerListView to render all items since it can never fill up the screen.

It will help if you can provide me with some more info. Few questions:

  1. Are you guys using this with React Native Web or ReactJS?
  2. Can you share the layout provider implementation?

@madnl
Copy link

madnl commented Aug 31, 2017

Hello,

Here's a sketch of how the scroller was tested:

import { View } from 'react-native';
import React from 'react';
import { RecyclerListView, DataProvider, LayoutProvider } from 'recyclerlistview';
import { array, func, node, number, string } from 'prop-types';
import createInstanceSelector from '../../modules/createInstanceSelector';
import ScrollPositionContext from './ScrollPositionContext';

const noop = () => {};

const positionCache = {};

class ScrollView extends React.Component {
  static propTypes = {
    assumedItemHeight: number,
    cacheKey: string.isRequired,
    items: array.isRequired,
    onAtEnd: func,
    onAtStart: func,
    onNearEnd: func,
    onNearStart: func,
    onPositionRestored: func.isRequired,
    renderer: func.isRequired
  };

  static defaultProps = {
    onPositionRestored: noop,
    onAtEnd: noop,
    onAtStart: noop,
    onNearEnd: noop,
    onNearStart: noop
  };


  static defaultProps = {}

  constructor(props, context) {
    super(props, context);
    this._layoutProvider = new LayoutProvider(
      (index) => this.props.items[index].type,
      (type, dim) => {
        dim.height = 200;
      }
    );
  }

  componentDidMount() {
    const { onPositionRestored } = this.props;
    // TODO: revisit
    (window.requestIdleCallback || window.requestAnimationFrame)(() => {
      onPositionRestored();
    });
  }

  render() {
    return (
      <RecyclerListView
        contextProvider={this._contextProvider()}
        layoutProvider={this._layoutProvider}
        dataProvider={this._dataProvider()}
        rowRenderer={this._rowRenderer}
        useWindowScroll
        forceNonDeterministicRendering
        onEndReached={this._handleOnEndReached}
        onEndReachedThreshold={500}
      />
    );
  }

  _rowRenderer = (type, data) => {
    const { renderer } = this.props;
    return renderer(data);
  };

  _handleOnEndReached = () => {
    const { onNearEnd } = this.props;
    onNearEnd();
  };

  _dataProvider = createInstanceSelector(this,
    (props) => props.items,
    (items) => {
      return new DataProvider((r1, r2) => r1 !== r2).cloneWithRows(items)
    }
  );

  _contextProvider = createInstanceSelector(this,
    (props) => props.cacheKey,
    (cacheKey) => new ScrollPositionContext(positionCache, cacheKey)
  );
}

export default ScrollView;

The context provider looks like this:

import { ContextProvider } from "recyclerlistview";

export default class ScrollPosition extends ContextProvider {
  constructor(cache, cacheKey) {
    super();
    this._cache = cache;
    this._cacheKey = cacheKey;
  }
 
  getUniqueKey() {
    return this._cacheKey;
  }

  save(key, value) {
    this._cache[key] = value;
  }

  get(key) {
    return this._cache[key];
  }

  remove(key) {
    delete this._cache[key];
  }
}

The layout provider is rather rudimentary - but we cannot know the sizes of items in advance.

@madnl
Copy link

madnl commented Aug 31, 2017

Regarding anchoring - that would probably require passing a keyFunction that assigns a unique key to each item. Otherwise it's very difficult to implement anchoring for lists where items can be arbitrarily added or removed.

Also, I'm curious if you're relying on the browser's scroll restoration to restore the position for lists. We're currently disabling that via history.scrollRestoration = 'manual'.

@naqvitalha
Copy link
Collaborator

Do not use useWindowScroll while using react native web. That is only required with ReactJS, with RNW scrollview will get used anyway.

Check out the following code:

/***
 Use this component inside your React Native Application.
 A simple scrollable list view
 */
import React from 'react';
import {RecyclerListView, DataProvider, LayoutProvider} from 'recyclerlistview';
import ScrollPositionContext from "./ScrollPosition";

const ViewTypes = {
    FULL: 0,
    HALF_LEFT: 1,
    HALF_RIGHT: 2,
};

let containerCount = 0;

class CellContainer extends React.Component {
    constructor(args) {
        super(args);
        this._containerId = containerCount++;
    }

    shouldComponentUpdate(newProps) {
        this.renderFromProps(newProps);
        return false;
    }

    componentDidMount() {
        if (this._idDiv) {
            this._idDiv.innerText = "Cell Id: " + this._containerId;
        }
        this.renderFromProps(this.props);
    }

    renderFromProps(props) {
        if (this._dataDiv) {
            this._dataDiv.innerText = "Data: " + props.data;
        }
    }

    render() {
        return <div {...this.props}>
            <div ref={x => this._dataDiv = x}/>
            <div ref={x => this._idDiv = x}/>
        </div>;
    }
}

/***
 * To test out just copy this component and render in you root component
 */
export default class RecycleTestComponent extends React.Component {
    constructor(args) {
        super(args);

        let width = window.innerWidth;
        this._contextProvider = new ScrollPositionContext();


        //Create the data provider and provide method which takes in two rows of data and return if those two are different or not.
        let dataProvider = new DataProvider((r1, r2) => {
            return r1 !== r2;
        });

        //Create the layout provider
        //First method: Given an index return the type of item e.g ListItemType1, ListItemType2 in case you have variety of items in your list/grid
        //Second: Given a type and object set the height and width for that type on given object
        //If you need data based check you can access your data provider here
        //You'll need data in most cases, we don't provide it by default to enable things like data virtualization in the future
        //NOTE: For complex lists LayoutProvider will also be complex it would then make sense to move it to a different file
        this._layoutProvider = new LayoutProvider(
            index => {
                if (index % 3 === 0) {
                    return ViewTypes.FULL;
                } else if (index % 3 === 1) {
                    return ViewTypes.HALF_LEFT;
                } else {
                    return ViewTypes.HALF_RIGHT;
                }
            },
            (type, dim) => {
                switch (type) {
                    case ViewTypes.HALF_LEFT:
                        dim.width = 150;
                        dim.height = 200;
                        break;
                    case ViewTypes.HALF_RIGHT:
                        dim.width = 150;
                        dim.height = 250;
                        break;
                    case ViewTypes.FULL:
                        dim.width = 300;
                        dim.height = 100;
                        break;
                    default:
                        dim.width = 0;
                        dim.height = 0;
                }
            }
        );

        this._rowRenderer = this._rowRenderer.bind(this);

        //Since component should always render once data has changed, make data provider part of the state
        this.state = {
            dataProvider: dataProvider.cloneWithRows(this._generateArray(1000)),
            isMounted: true
        };
    }

    _generateArray(n) {
        let arr = new Array(n);
        for (let i = 0; i < n; i++) {
            arr[i] = i;
        }
        return arr;
    }

    _rowRenderer(type, data) {
        //You can return any view here, CellContainer has no special significance
        switch (type) {
            case ViewTypes.HALF_LEFT:
                return (
                    <CellContainer data={data} style={styles.containerGridLeft}>
                    </CellContainer>
                );
            case ViewTypes.HALF_RIGHT:
                return (
                    <CellContainer data={data} style={styles.containerGridRight}>
                    </CellContainer>
                );
            case ViewTypes.FULL:
                return (
                    <CellContainer data={data} style={styles.container}>
                    </CellContainer>
                );
            default:
                return null;
        }
    }

    render() {
       return  <div style={{height:"100%", width:"100%", display:"flex"}} onClick={() => {
            this.setState({isMounted: !this.state.isMounted})
        }}>
            {this.state.isMounted?
            <RecyclerListView layoutProvider={this._layoutProvider} forceNonDeterministicRendering={true}
                              contextProvider={this._contextProvider}
                              dataProvider={this.state.dataProvider} rowRenderer={this._rowRenderer}/>: null}
        </div>
    }
}
const styles = {
    container: {
        justifyContent: 'space-around',
        alignItems: 'center',
        width: 200,
        height: 120,
        display: 'flex',
        flexDirection: 'column',
        border: '0.5px solid black',
        borderRadius: 20,
    },
    containerGridLeft: {
        justifyContent: 'space-around',
        alignItems: 'center',
        height: 260,
        width: 150,
        display: 'flex',
        flexDirection: 'column',
        border: '0.5px solid black',
        borderRadius: 20,
    },
    containerGridRight: {
        justifyContent: 'space-around',
        alignItems: 'center',
        height: 250,
        width: 150,
        display: 'flex',
        flexDirection: 'column',
        border: '0.5px solid black',
        borderRadius: 20,
    },
};

This is for ReactJS, context preservation works fine here. Just render this component in a window sized div and try out. Click will toggle mount/unmount. Also, we don't use browser scroll restoration. Context Provider caches layout values and maintains pixel perfect context on reload.

If this doesn't help I can try building one on top on RNW. Let me know.

@naqvitalha
Copy link
Collaborator

Also, in the example I posted check virtualization. 1000 items are being rendered on very few cells. Let's figure this out and post that may be we can also solve anchoring. Anyways, what is the use case for Twitter Lite?

@naqvitalha
Copy link
Collaborator

@madnl wrote about context preservation https://medium.com/@naqvitalha/context-preservation-on-page-destruction-in-react-native-and-web-using-recyclerlistview-8dd9e8475c3d . We can continue working on this thread if you're interested.

@tafelito
Copy link
Contributor

Hi, I have a similar requirement where I need to add items on top of the list and keep the scroll position and also having an inverted list that scrolls up. Instead of reaching the end would reach the start (or the end but inverted 😉 )

Are you planning on working on this?

@naqvitalha
Copy link
Collaborator

@tafelito If it's just inverted then it's easy. Like in a chat app you can just do a scaleY: -1 transform to get it to work. In case of twitter items can be added to the bottom of the list as well as the top. That is not supported yet. @surya-kanoria from our team is looking into making it possible. Async nature of react-native is making it quite tricky. A dedicated issue is open for that #44

@tafelito
Copy link
Contributor

Thanks @naqvitalha for the quick response. I tried using scaleY: -1 on the div container but it also requires to set the scale on the list as well. I tested this on chrome and it works but I didn't find a way to actually set a style to the listview.

Reagrding issue #44 I'll keep an eye on it.

Thanks

@naqvitalha
Copy link
Collaborator

Looks like in case of web style prop is not passed to the scroll container :( . But if you've set it on the enclosing div it should work. Individual items will also need to have the same transform.

Style not being applied is a bug. Open an issue and if you're looking into it a PR will be very helpful. Thanks!

@tafelito
Copy link
Contributor

yes that's why I did and it works fine. The only issue I see now is the scroll direction now it goes the opposite way on desktop. It makes sense but I need to find a way to invert that as well

I'll open a new issue for the styling.

Thanks

@naqvitalha
Copy link
Collaborator

@tafelito interesting, let me know if you're able to solve this. Very interested in the solution.

@tafelito
Copy link
Contributor

One way I can think of is handling it on the onWheel event, where you can check if you are inverting the list you force the scroll to the opposite direction (multiplying by -1 the scroll value on Y).

Ideally this should happen on the list itself as it would be easier to control it there, and then you could add an inverted property like RN FlatList does. I should check how they do it there, but that's my first thought

@naqvitalha
Copy link
Collaborator

FlatList does the same thing, uses transforms. The scroll direction is however unaffected by the transforms. It could be that native handles it in a different way. Scrolling in opposite direction could be something unique to web.

@tafelito
Copy link
Contributor

@naqvitalha I'll try to work on some version of this and I'll let you know how it goes!

@naqvitalha
Copy link
Collaborator

#44 will be used to track reverse list progress. Closing this since all other issues mentioned are resolved.

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

No branches or pull requests

4 participants