WICG / virtual-scroller

Other
2k stars 83 forks source link

Example hello world no longer works #115

Closed ebidel closed 6 years ago

ebidel commented 6 years ago

The example at https://valdrinkoshi.github.io/virtual-scroller/#example no longer works in the latest canary.

valdrinkoshi commented 6 years ago

Confirmed, that's because ItemSource uses the item itself to determine the element key. This causes the scroller to use the same dom element for all the data, in this case a single dom node that will render the last item in the visible range. If you scroll at the bottom of the list, you'll see the last item rendered as "199 -item".

This is interesting: what should the ItemSource do if the data contains the same object more than once?

Our current default is tailored for use cases where the array contains different data. If we want to play it safe, we should update ItemSource.fromArray to return the data index as element key instead of the item itself https://github.com/valdrinkoshi/virtual-scroller/blob/946c232a9d83582c77aa2f760dbd21bfd0d439c9/item-source.js#L38-L40 To be changed to

key(index) {
  return key ? key(array[index], index) : index;
}

@domenic WDYT?

domenic commented 6 years ago

Good catch. I remember this used to be index and then I changed it to item, because for cases like the contacts case, item is much nicer. (Reordering starts working automatically.)

I think it might be OK to go back to index. Then you basically just always need to supply a key, if you want good reordering behavior. Maybe that's fine.

An alternative is to be a bit magical, and if typeof === object || typeof === function && typeof !== null, use the item as the key. Otherwise use the index. I haven't thought this through too much yet.

valdrinkoshi commented 6 years ago

I'd go back to index, after all that's the only data the virtual-scroller deals with and can guarantee for. Doing the magical thingy would not work if there is the same object/function in the array, and would lead to difficult debugging for the user. It would be nice to be able to notify/warn the user about these cases when they happen, but I'm not sure VirtualScroller has a way to capture that.

domenic commented 6 years ago

OK, I'll prepare a PR.

valdrinkoshi commented 6 years ago

oh ok, I have one ready too if you want to enhance it

domenic commented 6 years ago

Oh, nevermind then, yeah, let's use yours!