feat(table): table virtualization #10182
Conversation
aborjinik
force-pushed
the
virtualizer-table-new
branch
3 times, most recently
from
29849dc
to
b322a2a
Compare
aborjinik
force-pushed
the
virtualizer-table-new
branch
from
b322a2a
to
dc974df
Compare
| @@ -210,6 +214,11 @@ class TableNavigation extends TableExtension { | |||
| this._gridWalker.setCurrent(eventOrigin); | |||
| } | |||
|
|
|||
| this._table._getVirtualizer()?._onKeyDown(e); | |||
There was a problem hiding this comment.
Maybe something for the future as an improvement. It would be nice to fire an event here instead, which would allow other components/extensions/features to possibly react to keydown events from TableNavigation.
Will also decouple TableNavigation from TableVirtualizer, which would be nice
This should be done in a follow up change/once we finalize the Virtualizer
| @@ -284,6 +293,7 @@ class TableNavigation extends TableExtension { | |||
| if (this._table.loading) { | |||
| this._table._loadingElement.focus(); | |||
| } else { | |||
| this._getNavigationItemsOfGrid(); | |||
There was a problem hiding this comment.
Why is this being called here and not used?
| @@ -115,7 +115,11 @@ class TableNavigation extends TableExtension { | |||
| } | |||
|
|
|||
| this._ignoreFocusIn = ignoreFocusIn; | |||
| element.focus(); | |||
| if (element === this._table._beforeElement || element === this._table._afterElement) { | |||
| element.focus({ preventScroll: true }); | |||
There was a problem hiding this comment.
Personal preference, but we could omit the if statement here:
element.focus({
preventScroll: element === this._table._beforeElement || element === this._table._afterElement
});|
|
||
| /** | ||
| * Fired when the virtualizer is changed by user interaction e.g. on scrolling. | ||
| * @param {first} number The 0-based index of the first children currently rendered |
There was a problem hiding this comment.
Shouldn't this be vice versa?
@param {number} firstSame goes for last
|
|
||
| /** | ||
| * Fired when the virtualizer is changed by user interaction e.g. on scrolling. | ||
| * |
| } | ||
|
|
||
| this._updateRowsHeight(); | ||
| if (this._tabBlockingState & TabBlocking.Released) { |
There was a problem hiding this comment.
This _tabBlockingState is extremely confusing. It is defined as an Enum, but you use it for bit operations.
I do get the idea (this only evaluating to true, if the blockingState is Next|Previous when it was released/invalidated), but on first sight it is extremely confusing.
We can keep these flags here (I do like bit shenanigans), but enum is misleading here a bit, as the _tabBlockingState is also able to take the value 5 or 6
There was a problem hiding this comment.
Maybe instead of _tabBlockingState being of type Enum, it is an explicit number representing bit flags instead (0b0000) and you instead of defining the Enum with integer values, you use binary instead?
enum TabBlocking {
None = 0,
Next = 1,
Previous = 0b10,
Released = 0b100
}| } | ||
|
|
||
| const scrollTop = this._scrollContainer.scrollTop; | ||
| const fixHeight = this._table.headerRow[0].offsetHeight; |
There was a problem hiding this comment.
This is only required if the header is sticky, right? So maybe this should be conditional
|
|
||
| _updateRowsHeight() { | ||
| const rowsHeight = this.rowCount * this.rowHeight; | ||
| this._rowsContainer.style.minHeight = `${rowsHeight}px`; |
| } | ||
|
|
||
| get _scrollContainer() { | ||
| return this._table!._tableElement; |
There was a problem hiding this comment.
Is this working for something like this?
<div style="height: 375px; overflow: auto">
<ui5-table>
<ui5-table-virtualizer row-height="51" row-count="1000" slot="features"></ui5-table-virtualizer>
</ui5-table>
</div>If that is not the case, we might need to explicitly document the restrictions
| }; | ||
|
|
||
| /** | ||
| * @class |
There was a problem hiding this comment.
It breaks if overflowMode=Popin. Maybe a validation would be nice here, that either throws an error or "corrects" the value to Scroll
Should be documented as well
The ui5-table-virtualizer component is used inside the ui5-table to virtualize the table rows.It is responsible for rendering only the rows that are visible in the viewport and updating them on scroll. This allows large numbers of rows to exist, but maintain high performance by only paying the cost for those that are currently visible.
This component is experimental.