feat: add final rowspan implementation #1101
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
[[row, cell, colspan, rowspan]]
for
loops and I got a little concerned when I saw afor
loop going down 3 level deep (scary😨)rowspan
or notSo 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 existingrowspan
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 arowspan
of 5 then the mapping that we'll keep internally would be{ 0: '1:6' }
... great but what if on top of therowspan
, we also have acolspan
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 arowspan
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 flagenableRowSpan
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 10rowspan
defined which is what the example demo has, so you can expect a little longer time spent on grids with a lot morerowspan
).