Closed ghiscoding closed 3 months ago
Run & review this pull request in StackBlitz Codeflow.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 99.8%. Comparing base (
c03b863
) to head (b542543
). Report is 13 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@zewa666 @jr01 CC'ing you guys since you are the biggest OData users I know 😉 I'm not sure if you would use this new feature or not but anyway feel free to comment. At this point, I'm not even sure I will merge the feature though I think it's worth it, what do you guys think?
~There's 1 thing that I'm unsure about, at the moment I have to call the gotoNextPage()
via the Example itself with a new onScrollEnd
event and the reason is because the OData Service doesn't have any DI and so it doesn't have access to the Pagination Service itself, hence why I need to advise the service to fetch the next batch of data (page) and so going to the next page for next fetch set of data seems like the only way to do it. I also don't want to add DI to the GridOdataService because that would require the user to add it himself which is not intuitive and would be a breaking change.~ Update: I actually found a way to move that code internally which is lot less confusing to the end user. The getCustomerCallback
object argument will now include a new infiniteScrollBottomHit
and when that hits, then end user will append data to dataset or initial load (not hit) then we assume new dataset (not appending)
this actually looks quite interesting. I'm not really sure it fits a lot of use-cases, as the drawbacks of not being able to share a position with a link plus leaving the location when sorting is somewhat a bummer (but understandable). Nevertheless it could be quite useful for some scenarios.
One thing I noticed is that UX wise it would be great if you'd have an indicator that there are more rows to load above/below from your current scroll location, which disappears once you hit the top or bottom.
this actually looks quite interesting. I'm not really sure it fits a lot of use-cases, as the drawbacks of not being able to share a position with a link plus leaving the location when sorting is somewhat a bummer (but understandable). Nevertheless it could be quite useful for some scenarios.
hmm yeah I wonder if Ag-Grid does better on their side, it's probably the same. Clearing the current data and restart is what Ag-Grid does too as per their description
The grid cannot do sorting or filtering for you, as it does not have all of the data. Sorting or filtering must be done on the server-side. For this reason, if the sort or filter changes, the grid will use the datasource to get the data again and provide the sort and filter state to you.
For your next question
One thing I noticed is that UX wise it would be great if you'd have an indicator that there are more rows to load above/below from your current scroll location, which disappears once you hit the top or bottom.
I actually had that in mind, the getCustomerCallback
returns an object to which I was thinking of adding another flag like infiniteScrollEndReached
, though I had an async problem. I'll take another look at it later. Also in theory since I'm using the Pagination Service, so I have all its info which could be used for displaying or whatever
Thanks for doing a quick review, I guess the use case would be to load data quickly like a list of news or articles while not caring about the position and/or rarely doing sorting (perhaps some users might disable it entirely to avoid full reload). For the Filtering however, I was thinking if I could just filter what is locally loaded at that moment and filter against that, if so I could use similar implementation as what I've done in the past with useLocalFiltering
in the Angular-Slickgrid Example 27 GraphQL Basic API without Pagination (that probably should be under a flag to use this approach or not).
EDIT
there you go, fully loaded it is... I also tested with useLocalFiltering
and after a small fix, it also works but only with what is being loaded in-memory (which is to be expected)
Hey @ghiscoding, I was on vacation past few weeks and couldn't respond earlier.
Really nice feature! Some minor remarks:
BackendService
handles UI concepts (scrolling, viewport, mousewheel). Perhaps that code can be moved to a more common place? https://github.com/ghiscoding/slickgrid-universal/blob/36bb193522aa71d0b5a57c1f391d3aec875c6032/packages/odata/src/services/grid-odata.service.ts#L108) Not exactly sure what you mean about the drag block, but the way I've done it is to simply calculate the end of the scroll by its height and scrollY position, and it should work with any kind of scroll as long as you reach the bottom of the scroll.
For the memory usage, I assume that it's normal since that demo that I created keeps adding items to the DataView, so the memory usage will keep increasing but it should be equal to a static datagrid with the same number of rows and Infinite Scroll is more of a UI concept that doesn't always fit the reality lol. We can blame the guys who created the term "Infinite Scroll" 😆 ... and in case that you might say "but shouldn't we also remove items when adding more items to keep memory usage lower?", to which I would reply, that should be implemented on the userland side, in the demos I'm using DataView.addItems()
, the user could choose to remove older items if he/she wishes too.
Lastly for the UI code, I think I tried to put that code in the UI component but I had some issues when I tried at the beginning, I can take another look to see if it's possible again now that it's all connected, that would indeed simplify the code on all Backend Services (you're probably right in the sense that there's no need to duplicate this code in all backend services)
Thanks for the feedback, it came at a good time since I was expecting to publish tonight after work :)
and there you go, I was able to move all the Infinite Scroll logic to the vanilla component (and Angular-Slickgrid, ...) in PR #1632 That was a great suggestion that I had forgot to review after doing the POC, so thanks again for the quick review
Similar approach to Ag-Grid Infinite Scrolling while also deviating a bit from Ag-Grid's approach
gotoNextPage()
presets.pagination
is not supported with Infinite Scroll and will revert to the first page when presets are provided. The reason is simply because, since we keep appending the data, it always has to start on first page or else we'll be missing some data.Why and when would we use it?
TODOs
dataView.addItems()
directly or if Slickgrid-Universal gets out of sync?