On Demand Scrolling #39
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39 +/- ##
===========================================
- Coverage 98.17% 62.03% -36.14%
===========================================
Files 10 10
Lines 219 461 +242
Branches 34 109 +75
===========================================
+ Hits 215 286 +71
- Misses 0 144 +144
- Partials 4 31 +27
Continue to review full report at Codecov.
|
| @@ -8,7 +8,7 @@ import DataProviderBase from './bases/DataProviderBase'; | |||
| import Body from './Body'; | |||
| import GridRegistry, { gridRegistry } from './GridRegistry'; | |||
| import Header from './Header'; | |||
| import { DataProperties, HasColumns, SortRequestListener } from './interfaces'; | |||
| import { DataProperties, HasBufferRows, HasColumns, HasScrollTo, ScrollToDetails, SliceRequestListener, SortRequestListener } from './interfaces'; | |||
There was a problem hiding this comment.
Looks like some unintentional double spaces in here.
| @@ -101,6 +107,18 @@ abstract class DataProviderBase<T = object, O extends DataProviderOptions = Data | |||
| } | |||
|
|
|||
| /** | |||
| * Apply a slice to the data that would be returned | |||
| * otherwise. Can also be assigned from | |||
|
|
||
| let rowHeight = 0; | ||
| let rowCount = 0; | ||
| for (let details of from(itemElementMap.values())) { |
There was a problem hiding this comment.
Does itemElementMap contain collapsed rows in a tree scenario, and would they have an element that could mess up the average height if that is the case? Not sure if we're even worried about tree stuff for this PR.
| classes: this.classes(bodyClasses.content) | ||
| key | ||
| }, [ | ||
| w<Row>('row', { |
There was a problem hiding this comment.
Why is it necessary to wrap each row in a div?
| * Based on what is currently rendered, find the average height | ||
| * of each row and, if nothing is rendered, return 20. | ||
| */ | ||
| protected estimatedRowHeight(): number { |
There was a problem hiding this comment.
Might be better if this is a property with get defined, or rename the method to getEstimatedRowHeight
| * Uses the height of the scroll area and the estimated row height | ||
| * to estimate the number of rows that will fill it | ||
| */ | ||
| protected estimatedRowCount(): number { |
There was a problem hiding this comment.
Might be better if this is a property with get defined, or rename the method to getEstimatedRowCount
|
|
||
| let rowHeight = 0; | ||
| let rowCount = 0; | ||
| for (let details of from(itemElementMap.values())) { |
There was a problem hiding this comment.
A pain point I discovered in working with this same logic in dgrid 1 is that browsers do not always give reliable information about element dimensions, most notably when there is any scaling in effect (e.g. a high dpi display with OS-provided scaling, or zoom level set in the browser).
Inspecting a node in the dev tools would give a non-integer value, e.g. 25.57, whereas element.offsetHeight would report 25. This error accumulated over a large number of rows results in significant height discrepancies. I found it more accurate to get height values directly from the node containing all the rows, rather than summing up the heights of each individual row.
| @@ -1,44 +1,590 @@ | |||
| import { from, includes } from '@dojo/shim/array'; | |||
There was a problem hiding this comment.
Have we developed any standard for this? Had any discussion? Personally I rather dislike importing and calling from directly. It doesn't make for clear code. I'd prefer:
array.from
or
import { from as toArray } from '@dojo/shim/array';
| }, children); | ||
| } | ||
|
|
||
| protected visibleKeys() { |
There was a problem hiding this comment.
Might be better if this is a property with get defined, or rename the method to getVisibleKeys
| break; | ||
| } | ||
| } | ||
| for (const renderedDetails of allDetails.reverse()) { |
There was a problem hiding this comment.
Might calling reverse impact performance?
| } = {} | ||
| } | ||
| } = this; | ||
| const max = (dataLength > 0 ? dataLength - 1 : 0); |
There was a problem hiding this comment.
max what? Could this benefit from a more descriptive name?
|
|
||
| // Use previous keys to watch for deleted items | ||
| // and insert new keys at the indexes detected above | ||
| for (let i = 0, il = previousKeys.length; i <= il; i++) { |
There was a problem hiding this comment.
Can we use a more descriptive name than il? Perhaps i as well given the length and complexity of this loop.
| sort?: SortDetails[]; | ||
| } | ||
|
|
||
| export interface HasBufferRows { | ||
| /** | ||
| * @default 10 |
There was a problem hiding this comment.
This seems like not the right place to document default values - is it possible to document them in the right place and get the right generated doc output?
Type: feature
The following has been addressed in the PR:
Description:
Resolves #21
This introduces the slice API to data providers to request the data only contain a subsection of the currently available data the provider would otherwise return (including sorts etc.)
The grid body makes a slice request to the data provider that will contain enough data to fill the content area as well as buffer rows above and below it.
As rows are added and removed from the grid, their heights are factored into the scroll position of the scrollable area to ensure that as the size of the scrollable content changes, the scroll position remains at the same position within the visible content.
It adds a large margin before and after the content area so that rapid scrolling won't "hit an edge".
If scrolling is done so rapidly that the user ends up outside the buffer rows, the grid body estimates how far it has been scrolled beyond the buffer rows, makes a slice for that data, and scrolls to the estimated row.
Users can also define what index to scroll the data to through a grid property.