feat: create virtual scroller class #70
Conversation
samrichardsontylertech
added
enhancement
There was a problem hiding this comment.
This is looking great so far! Nice work. I don't have a lot of feedback on the logic itself yet, as I more or less skimmed the calculations and flow of the more complex parts. I was focusing more on architectural stuff, APIs, performance concerns, potential APIs, and code organization for this first pass.
I think from a users perspective the functionality works as expected, and adjusting the buffer worked really to get the desired result. For a POC this is in a really good place.
I know some of my comments are a little out of context in regards to out goal of keeping this scoped to internal use within our dropdowns, but I wanted to express them while they came to mind so feel free to ignore certain things.
I say the next steps should be to:
- cleanup and organize code
- finish todo items
- apply this to the list-dropdown and iterate as necessary (this will help reviewers play with it in storybook as well)
- write some unit tests around it, as well as test smaller utilities in isolation (as noted in my code organization comments), and that should help flush out any remaining issues once the code is more finalized
| private _createChildElement(index: number): HTMLElement { | ||
| const element = this._childBuilder(this._data[index], index); | ||
| element.style.position = 'absolute'; | ||
| element.style.left = '0'; | ||
| element.style.top = this._getChildTop(index); | ||
| if (!this._skipAccessibility) { | ||
| toggleAttribute(element, true, 'aria-setsize', this._setSize.toString()); | ||
| toggleAttribute(element, true, 'aria-posinset', (index + 1).toString()); | ||
| } | ||
| return element; | ||
| } |
There was a problem hiding this comment.
We may want to implement a rolling cache here to avoid having to always call the builder. We could use index by default for the cache key to start for our dropdown use case since the order doesn't change. In the future it may also be worth it to implement an alternative API for returning results from the builder callback to allow devs to not only provide the element itself, but also a "tracking" key that maps the element to a specific key to avoid ordering issues.
There was a problem hiding this comment.
Caching/reusing the created elements might also help for some of the problems we've seen with other virtualization libraries for something like a checkbox list, where if you check an item, scroll it off screen and then scroll back to it, the box appears unchecked. That's maybe a bad example because that checkbox state should probably be tracked somewhere beside the element itself and referenced by the builder, but I can't recall the exact scenario we ran into. Just that in addition to performance, it might also avert a whole class of virtualization-specific problems for developers.
| this._applyContainer(container); | ||
| this._data = [...data]; | ||
| this._childBuilder = childBuilder; | ||
| this._childHeight = childHeight; |
There was a problem hiding this comment.
We should potentially adjust the builder callback to expect a result object in the following form:
// Adjust builder type to allow for also returning string and IVirtualScrollerChildBuilderResult
type VirtualScrollerChildBuilder<T> = (data: T, index: number) => HTMLElement | string | IVirtualScrollerChildBuilderResult;
// Accept a new result from the builder with more (optional) info
interface IVirtualScrollerChildBuilderResult {
element: HTMLElement | string; // The element instance
height?: number; // To allow for dynamic height that overrides global `childHeight`
trackBy?: () => unknown; // An optional
}This could allow us to use dynamic height elements that the developer can calculate/provide to use to avoid having to calculate it on the fly.
| private _createChildElement(index: number): HTMLElement { | ||
| const element = this._childBuilder(this._data[index], index); | ||
| element.style.position = 'absolute'; | ||
| element.style.left = '0'; | ||
| element.style.top = this._getChildTop(index); | ||
| if (!this._skipAccessibility) { | ||
| toggleAttribute(element, true, 'aria-setsize', this._setSize.toString()); | ||
| toggleAttribute(element, true, 'aria-posinset', (index + 1).toString()); | ||
| } | ||
| return element; | ||
| } |
There was a problem hiding this comment.
Caching/reusing the created elements might also help for some of the problems we've seen with other virtualization libraries for something like a checkbox list, where if you check an item, scroll it off screen and then scroll back to it, the box appears unchecked. That's maybe a bad example because that checkbox state should probably be tracked somewhere beside the element itself and referenced by the builder, but I can't recall the exact scenario we ran into. Just that in addition to performance, it might also avert a whole class of virtualization-specific problems for developers.
|
It's been a while since my last update on this. I addressed most of the points raised here in the latest commit, cleaned up and commented the code and enabled a couple of properties that were stubbed out earlier. There are still some cross-browser performance issues that need to be addressed but I think we have a decent first iteration as far as functionality goes. Next up we can think about:
But in the meantime we can work on tests and implementing this into a few components. |
There was a problem hiding this comment.
Overall things are really coming along nicely. I like the configuration and comments that were added to the implementation. Much easier to follow now and reason about.
I noticed that when trying to scroll the container by dragging the mouse in Safari didn't work, but I'm sure you're aware of that one?
Otherwise, I think we should update the list-dropdown next to use this and that should help us uncover any other items we need to take into account.
If you'd like to get us together to discuss your remaining questions from the previous comment feel free to put something on the calendar.
DRiFTy17
force-pushed
the
feat/virtual-scroller
branch
from
39688ba to
da79fd4
Compare
|
Using VoiceOver in Safari and Chrome, only the elements that were in the scroller when it was navigated to are announced with their positions and the set size (e.g. "4 of 20"). It also seems to be ignoring the Probably related to this. |
This seems to be an issue with VoiceOver specifically. I submitted a bug report to Apple so we'll see if/when it gets addressed. In the meantime there isn't much to do about it, the accessibility features here are already up to spec. |
PR Checklist
Please check if your PR fulfills the following requirements:
Describe the new behavior?
A virtual scroller class was created to append and remove elements as they come into view. A container element, data array, element builder callback, and fixed item height are required to instantiate it. To maintain semantic relationships, no intermediate elements are created but an
aria-hiddensibling to the items is added to set the scroll height.Additional information
This is a draft pull request. The virtual scroller's name, location in the code, and exposure as part of the public API can all be changed. The main intention for this at the moment is to be used internally as part of list dropdowns.