implement scrollToIndex() #97
Conversation
README.md
Outdated
| @@ -97,6 +97,17 @@ This re-renders all of the currently-displayed elements, updating them from thei | |||
|
|
|||
| This generally needs to be called any time the data to be displayed changes. This includes additions, removals, and modifications to the data. See our [examples below](#data-manipulation-using-itemschanged) for more information. | |||
|
|
|||
| ### `scrollToIndex(index: number, position: string = "start")` method | |||
There was a problem hiding this comment.
maybe rename position to viewportPosition? @domenic wdyt?
There was a problem hiding this comment.
It feels a bit like the align properties for flexbox. The position here also depends on the scroll direction.
Maybe you want to change the names of the values instead? position: "viewport-start" or position: "scroll-direction-start"
There was a problem hiding this comment.
I see that scrollIntoView uses "start", "center", "end", or "nearest" so let's keep that
alignToTop: If true, the top of the element will be aligned to the top of the visible area of the scrollable ancestor. Corresponds to scrollIntoViewOptions: {block: "start", inline: "nearest"}. This is the default value.
Here the platform is using block and inline which is more a what than position. It seems a bit more future proof to create a scrollToIndexOptions dictionary
There was a problem hiding this comment.
+1 on using an options dictionary. I didn't realize that there was also a "behavior" member (hidden in https://drafts.csswg.org/cssom-view/#dictdef-scrolloptions) which we may want to add in the future.
That still leaves the question of what to call it. I think "position" is pretty good though. "Scroll to index 5 at the end position" sounds OK when said out loud.
(Working on updating this now.)
DESIGN.md
Outdated
| layout.scrollToIndex(19, 'end'); | ||
|
|
||
| // Scroll to the 100th item, position it at the end of the viewport | ||
| // if we are scrolled above it already, otherwisef position it to the start. |
DESIGN.md
Outdated
|
|
||
| // Scroll to the 100th item, position it at the end of the viewport | ||
| // if we are scrolled above it already, otherwisef position it to the start. | ||
| layout.scrollToIndex(99, 'nearest'); |
There was a problem hiding this comment.
Are these supposed to scroll abruptly - or do some smooth scrolling, like scrollIntoView on the web platform can do?
https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView
There was a problem hiding this comment.
abruptly for now, i'm not sure how to implement smooth scrolling...probably we should move this method into VirtualScroller, which could check if it has the dom for the desired index, and call the native scrollIntoView..!
demo/scrolling.html
Outdated
| .then((resp) => resp.json()) | ||
| .then((contacts) => virtualScroller.itemSource = contacts); | ||
|
|
||
| window.scrollToIndex = function(index, position) { |
There was a problem hiding this comment.
Any reason why you mix arrow functions and non arrow functions?
There was a problem hiding this comment.
not really, i can change this to arrow function 👌
There was a problem hiding this comment.
Made some tweaks. Overall looks great, although I know you had more work you might want to do; happy to re-review later.
I added a TODO about how we should probably handle out-of-range indices ourselves; currently it's handled in the demo. Based on how the other DOM scrolling methods work, it seems like it should clamp to 0/max number of items.
|
I'll merge this as is, we can add "behavior" implementation in a separate PR |
Fixes #88 by providing
scrollToIndex()method, documentation and demo. This works both with all layouts we ship.@domenic feel free to update the README.md as you see fit.
We invalidate the
_scrollToIndexonly when receiving scroll position changes (in other words, when user scrolls).This implementation differs from #83 in the following: we don't compute the scroll anchor when user scrolls, nor allow customizing anchoring via itemAnchor and viewportAnchor.