Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VirtualScroll Feature #123

Closed
wants to merge 3 commits into from
Closed

VirtualScroll Feature #123

wants to merge 3 commits into from

Conversation

gem85247
Copy link

@gem85247 gem85247 commented Mar 23, 2023

Hi,

First of all, a big thanks for creating and maintaining this package, I really love how you improved the original ng2-smart-table package!

I'm submitting a pr because I managed to implement a working virtual scroll using the official @angular/cdk (https://v13.material.angular.io/cdk/scrolling/overview). I tested the "angular cdk" library from version 13.x till 15.x against this library.

I also added a demonstration to the demo page (Examples => Various => Virtual Scroll). There you will see the virtual scroll in action. In a nutshell, no matter how many data entries you have, the DOM will only create a fixed amount of HTML table rows (<tr>) depending on how big your viewport is (you can view this in any browser DOM inspector).

This ensures that the browser DOM doesn't get overloaded with html elements that aren't even visible to the user. This feature will especially be helpful for people who are feeding big datasets to their users. I personally use this in a combination with an infinite scrolling list that loads more data as the user scrolls down and it seems to be working fine.

Let me know what you guys think of this feature and I'm happy to answer any questions or provide more information as needed.

Thank you for your time & consideration!

@uap-universe uap-universe self-assigned this Mar 27, 2023
@uap-universe
Copy link
Collaborator

Although I announced that I want to retreat from this project in #116 I am quite amazed by this feature. I will have a look. And when I am already "in" again, I will also properly define a version 2.9.0 milestone and include some of the open bugs. Give me some days, though, I have quite a busy schedule.

Just one short question, before I start looking at the details: have you tested this with both the local and the server data source? What is the technique for fetching remote data in the presence of virtual scrolling?

@uap-universe uap-universe added the enhancement New feature or request label Mar 27, 2023
@uap-universe uap-universe self-requested a review March 27, 2023 15:23
@gem85247
Copy link
Author

Sure man, take your time!

To answer your question: No I haven't checked this with the ServerDataSource variant, let me check this first so we don't waste each others time ;)

chore: update demo pages + add documentation to VirtualScroll & InfiniteScroll settings
@gem85247
Copy link
Author

Hi again,

I updated the demo pages and added a ServerDataSource implementation.

I also added a new feature "InfiniteScroll" which is an additional sub-setting you can give to the "VirtualScroll" setting. I added some explanation to the setting parameters so you (or anyone for that matter) can better understand what the implications are of each setting.

The ServerDataSource works fine for "VirtualScroll" without "InfiniteScroll". But when we are adding data to the ServerDataSource with the "InfiniteScroll" set-up, it seems to not visually show the (lazy) appended data. When I look inside the internal data array, the added data is present, but as I stated it is not visually shown. Might be a ChangeDetection problem I think?

Let me know if you ran into this issue before, if not I wil continue to try and debug it.
Everything else is working fine though!

Best Regards

chore: cleanup logging + make getNextFunction required
@gem85247
Copy link
Author

Ok so this is the final addition (I think) 😁

I added some loading placeholders when the next batch of data is being fetched. To enable this behaviour you need to set the loadingPlaceholder field to a number higher than 0.

Finally I did some cleanup and made the getNextFunction required in the "InfiniteScroll" settings interface.

Let me know what you think!

@uap-universe uap-universe added this to the v2.9.0 milestone Mar 30, 2023
export class NumberToArrayPipe implements PipeTransform {

public transform(value: number | undefined):any[] {
return new Array(value || 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use ?? operator.

);
}

private getNextBatchServer(offset: number): Observable<any[]> {
Copy link
Collaborator

@uap-universe uap-universe Apr 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand this function. What is it supposed to do? It is never called, until you reach the end of the table. Also, the "fake data" is not coming from the server, which looks like quite disconnected from what a user would actually need to do. So I don't think this serves well as an example.

The second observation is: when the end of the table (i.e. after 5000 entries) is reached, 20 identical HTTP requests are sent out (https://jsonplaceholder.typicode.com/photos?_sort=&_order=&_page=1&_limit=9007199254740991).

Maybe the infinite scroll is not working as inteded. Please check.

Update: the limit of 9007199254740991 looks like the perPage setting you have specified. Just as a side note: this might break server implementations that only support 32 bit integers.

Copy link
Collaborator

@uap-universe uap-universe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gem85247 Please consider the comments in the respective files and also please add all new settings to the documentation.component.html file. Beyond that, I think this can be merged soon.

@uap-universe
Copy link
Collaborator

uap-universe commented Apr 18, 2023

@gem85247 Another thing I have noticed is that expanded rows or rows that are currently edited inline are reset when the next batch is fetched in the infinite scroll example with local data source. Also, the feature does not play well with multi selection. When you check the "select all" checkbox, only the rows that are currently loaded are selected. When you scroll down and fetch the next batches, the items are not selected.

@uap-universe uap-universe removed this from the v2.9.0 milestone May 16, 2023
@uap-universe
Copy link
Collaborator

Closed due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants