search

SitePen / dgrid

A lightweight, mobile-ready, data-driven, modular grid widget designed for use with dstore
http://dgrid.io/
Other
628 stars 295 forks source link

OnDemandList + Tree: refresh with keepScrollPosition not working #1316

Open edhager opened 8 years ago

edhager commented 8 years ago

With the latest code in master, follow these steps:

  1. Load the test/Rest.html manual test page.
  2. Expand the first row.
  3. Expand the first child of the first row.
  4. Scroll down until the row with id 0-0-242 is the top visible row.
  5. In the console, enter "grid.refresh({keepScrollPosition: true});"

The grid will refresh with the top level row 244 at the top of the visible area. Scrolling works fine and if you scroll to the top, the rows will expand as they should. So, this is not a critical bug, but it could be very annoying for users.

I added code that delays the expansion of rows until each parent row has been inserted into the DOM (e1cbc84460c6bb4e4e36a2c404729d1bceb20069). That fixed a problem with refresh() completely breaking OnDemandLIst with expanded rows. But now I am afraid the code that restores the scroll position is running before all of the rows are expanded.

kfranqueiro commented 8 years ago

Is this sort of a new incarnation of #403?

edhager commented 8 years ago

@kfranqueiro, maybe.

I "fixed" this issue for now in fbd8568210a6d9e943e10250be62a446984f26b6 by disabling the keepScrollPosition functionality in the Tree mixin if there are expanded rows. I explained the reason here: https://github.com/SitePen/dgrid/blob/master/Tree.js#L315-L319. Basically, after making a scroll position jump, the code doesn't know exactly which rows should be rendered at that position. All of the expand rows (including nested expanded rows) from the top to that position would need to be loaded and expanded to calculate heights to decide which rows are visible.

With the old not-pruning-child-rows code, we may have been able to get this to work but that is because the grid would have loaded all of the rows all the way down to the scroll position. In a large grid, that would have been a performance hit and it would have used a lot of memory.

Maybe the _expanded property could be used to determine which rows are visible at the desired scroll-to position. More info could be added to the _expanded map like expanded height. But sorting would need to be factored in.

There is a desire for a scrollTo method that accepts a row id or and an item id. The Tree mixin would have the same issue trying to figure out which rows are visible.

I'm definitely open to ideas.