-
Notifications
You must be signed in to change notification settings - Fork 426
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
Conversation
Interesting. I'm also in the process of working on a variable row height feature which approaches things in a not dissimilar way. |
@6pac well actually that first commit from GerHobbelt's fork also included variable row height and it was working for the most part... but I removed it (or didn't include it) because it brought a bunch of different problems. The biggest challenge with If you go on GerHobbelt's fork, clone it and open his Example 31 with Note that I added a bunch of new cell navigation shortcuts, and a bunch of new Cypress E2E tests as well, in recent PR #1093 (i.e.: |
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
).