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

feat: add final rowspan implementation #1101

Merged
merged 6 commits into from
Jan 18, 2025
Merged

feat: add final rowspan implementation #1101

merged 6 commits into from
Jan 18, 2025

Conversation

ghiscoding
Copy link
Collaborator

@ghiscoding ghiscoding commented Jan 18, 2025

closes #614
closes #381

So I've been working over a month on this, initially based on the @GerHobbelt's fork from his initial rowspan commit and after a few bug fixes, I was surprised to see it working at least for the most part but it had particular negative side effects that made me rethink if I could improve it further. I had a few problems with that first approach and they are mostly all related to each other:

  1. it required to loop through the entire dataset to build a 2D array of every cells of that entire dataset with a 2D array that keeps [[row, cell, colspan, rowspan]]
    • it had a few nested for loops and I got a little concerned when I saw a for loop going down 3 level deep (scary😨)
  2. this 2D array mapping was being built for every grid regardless if you were using rowspan or not
  3. and finally it required to keep this large 2D array in memory (again for every grid)

So I decided to see if I could do it in a different way that would require less of a footprint (no 2D array) and less intrusive (it shouldn't impact regular grids perf at all) and I came up with this new approach. Instead of creating this large 2D array, what if we keep only the start/end indexes of each rowspan but instead of saving them by rows, we keep them by column indexes (basically the inverse of item metadata because that is row based)... and that is what I went with, the perf is quite good (about twice as fast as the first approach above). Note that this 2nd approach still does require to loop through all dataset rows to extract every existing rowspan from all item metadata but it does it in a much quicker way. For example, let's say that on the 1st column from the 2nd row, we have a rowspan of 5 then the mapping that we'll keep internally would be { 0: '1:6' }... great but what if on top of the rowspan, we also have a colspan of 2 on the same cell? Then our mapping becomes { 0: '1:6', 1: '1:6' } (basically duplicating the same start/end on the 2nd column as well). Also another concept is that if any cell has a rowspan child that becomes out of the viewport, we need to keep its parent rendered in the DOM at least until none of it is no longer shown at all (because the parent cell, is the cell holding the height for the entire rowspan). Then when if its the cell parent & children no longer intersect in the viewport then we can remove it from our cache (basically this will keep certain data rows a little longer in the DOM but that is of course necessary for the rowspan to work properly, i.e. our example has a rowspan that spreads across the grid until the bottom, so that parent row is basically never being removed from the cache because that one in particular must be kept for it to show correctly)

So why is this new approach better than the first approach? Well this approach takes a lot less space in memory and if our rowspan is very large, let say that it spans over 100 rows, then the mapping is nearly the same and we just need to update our ending for example { 0: '5:105' } (6th row, spanning for 100 rows, so 100+5=>105 is the ending)... simple! The other bonus is that with this new approach, I do have to keep a mapping object but it's relatively small and the bonus is that I've put this under a grid option flag enableRowSpan so that regular grids have 0 perf impact as opposed to the previous approach that impacted all grids (but just to be clear though, the perf impact wasn't that huge, for 500K rows it was around 300-400ms to build mapping for the 1st approach and around 100-200ms for new approach and also note that it was assuming only about 10 rowspan defined which is what the example demo has, so you can expect a little longer time spent on grids with a lot more rowspan).

image

image

@ghiscoding ghiscoding changed the title Feat/row span feat: add final rowspan implementation Jan 18, 2025
@ghiscoding ghiscoding merged commit 2e65fa8 into master Jan 18, 2025
2 checks passed
@ghiscoding ghiscoding deleted the feat/row-span branch January 18, 2025 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there an example of cell merging? Rowspan
1 participant