Header not scrolling correctly in next branch without jQuery

[6pac]

As per GIF, header row is not scrolling. In the current master it works, but not in next. I've spent about half an hour looking, but can't find the issue. @ghiscoding or @MarkoBL

[6pac]

(note this issue was occurring before my recent $.each() fix push)

[ghiscoding]

good catch, I'll take a look and come up with a fix, it's probably related to the scroll event not propagating.

As for the $.each(), I tried to find any of them with a return in slick.grid and there's only 6 of them and none seem to have a return. Do we need to check also for .forEach? I wonder if the .forEach works with return instead of continue, my guess would be that forEach requires a return since I think continue are reserved to for loops only

[ghiscoding]

fixed in commit 98aeb9b, there was a header-row scroll missing. It should be ok for all other elements, but it would be nice to keep that in mind to make all other elements works too.

I'm nearly all done with Controls & Plugins, there's only the Pager (pagination) plugin left to do. I do however want to also review all the code that Marko added as utils.template('<div>...</div>') and I plan to replace them all with proper DOM element creation, because as it currently stands the utils.template uses innerHTML to create these elements and I don't like that, it's a lot less efficient compare to pure element creation (document.create) and also prone to possible XSS attack (remember the sanitize html saga...), so anyway I intend to replace them all with pure JS and that could also improve the grid performance.

[ghiscoding]

Reopening since I fixed it for regular grid but it's also totally broken for frozen grids

[ghiscoding]

fixed again by commit 2405fe6

[ghiscoding]

I'm also done migrating all Controls & Plugins, so at this point it's just code cleaning and making sure everything works as expected. I still want to replace all these utils.template('<div>...</div>') that I mentioned in previous comment since these are using innerHTML and I want to replace with native code

[6pac]

Awesome. Yep, element creation sounds much better. I'd be looking at the jQuery source to see what they do there.

FYI I went over slick.grid.js with a fine tooth comb for the .each() issue and committed direct to next. This should all be good now.
Yes, .foreach should use return, and for (let i = 0; i < children.length; i++) { should use continue.

While we're on a roll, #737 still appears to be a problem. Happy to look, but you just seem to be 'in the zone' right now, so I suspect what might take me several hours you may be able to do in 15 minutes.

[6pac]

nb. some of the $.each were converted to .forEach() and some were converted to for() iterations. I went back to the master branch, looked for .each and then looked at what it had been converted to to make sure the paradigms were correct.

[ghiscoding]

While we're on a roll, #737 still appears to be a problem. Happy to look, but you just seem to be 'in the zone' right now, so I suspect what might take me several hours you may be able to do in 15 minutes.

I think this one is cause by the bottom container not being entirely filled except when you start scrolling then it adds more slick rows, a potential fix would be trigger a scroll event at same position when frozen row is changed to bottom.

Actually taking a quick look at it, I see that the problem is because the height is not recalculated properly after changing the frozen row dynamically. If you use this css style height: 1.24988e+06px; width: 1040px; (which is a copy of the grid-canvas grid-canvas-top grid-canvas-left and you apply it to the bottom (after changing frozen row that is), it will work as expected

from this

to this

the problem is I don't know how that height is being calculated correctly when we initialize the grid but not after. A potential quick hack fix would be to copy the one from the top into the bottom and vice versa when you do the inverse but that has a potential issue when user also changes the number of frozen rows (not just top/bottom toggle).... so anyway, I have identified the issue but not a real fix 😝

I actually wonder if that height that I wrote about is actually these ones from the default options

SlickGrid/slick.grid.js

Lines 115 to 116 in 21fe941

	ffMaxSupportedCssHeight: 6000000,
	maxSupportedCssHeight: 1000000000,

[6pac]

Thanks, that helps a lot. Will see what I can do