Improve the vertical scrolling of our treeviews#4332
Conversation
| ); | ||
| } | ||
|
|
||
| _scrollContainerHorizontally(container: HTMLDivElement, offsetX: CssPixels) { |
There was a problem hiding this comment.
Note: the offsetX we pass here is usually wrong because it doensn't take into account the fixed column widths. I intend to change this once #4204 is done.
Codecov ReportBase: 88.34% // Head: 88.33% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4332 +/- ##
==========================================
- Coverage 88.34% 88.33% -0.02%
==========================================
Files 282 282
Lines 25302 25229 -73
Branches 6817 6797 -20
==========================================
- Hits 22354 22285 -69
+ Misses 2735 2731 -4
Partials 213 213
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I like it! |
|
I had a look at tests, but the VirtualList component is very difficult to test with our current infrastructure using jsdom, because we don't have Layout nor scrolling. Therefore I don't think I can do useful tests for this PR :/ |
3620a67 to
92e2905
Compare
|
|
||
| let scrollMargin = 3 * this.props.itemHeight; | ||
| if (container.clientHeight < 2 * scrollMargin) { | ||
| // The container is too small to use a margin. |
| itemTop + bigJump < container.scrollTop || | ||
| itemBottom - bigJump > container.scrollTop + container.clientHeight | ||
| ) { | ||
| const scrollTopToCenterItem = |
There was a problem hiding this comment.
nit: it would ne nice to explain why we have this case in a comment for future reference.
92e2905 to
fdcf5d4
Compare
This makes the following changes:
production
deploy preview
I considered having a different generation state to distinguish centering vs simply navigating, but this is more changes that I wanted to do now. Instead I used an heuristic to detect the "jumps": that's happening when it's more than 16 times the item height (navigating with page down/page up moves by 15 items, so the number was chosen for this). It's not perfect because sometimes when clicking repeatedly in the timeline we don't always trigger this heuristic. But this is right in most times.