-
Notifications
You must be signed in to change notification settings - Fork 423
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
Problem with updating frozenColumn via setOptions #1038
Comments
That's not a problem with the library but rather something that must be implemented by the user (you). Anytime something changes that could affect the pre-header columns, you have to re-render the pre-header and it really should be handled by you not by the library itself. For example in Slickgrid-Universal, I have a separate service that subscribes to a bunch of these potential event that affects the pre-header and the Slickgrid-Universal lines of code ref // and finally we need to re-create after user calls the Grid "setOptions" when changing from regular to frozen grid (and vice versa)
this._eventHandler.subscribe(grid.onSetOptions, (_e, args) => {
// when user changes frozen columns dynamically (e.g. from header menu), we need to re-render the pre-header of the grouping titles
if (args?.optionsBefore?.frozenColumn !== args?.optionsAfter?.frozenColumn) {
this.delayRenderPreHeaderRowGroupingTitles(0);
}
}); If we look at the existing example, we can see similar approach but it currently only re-render the pre-header for column resize and/or column reorder, however SlickGrid/examples/example-draggable-header-grouping.html Lines 543 to 548 in 8d4a290
So in summary, I don't think we have a demo that has both Frozen enabled and Header Grouping (pre-header), but if we do then we should add the same kind of code logic in that demo... so again, this must be handled on the userland (you) not the library itself |
This has nothing to do with additional pre-header row though. Even with a vanilla grid and with no column grouping nor frozen column enabled, calling setOptions alone would make all the viewports lose their horizontal scrolling positions because of this check
If I understand it correctly, the default frozenColumn is -1 instead of 0, so that branch is always taken even if there is no frozen column. Actually, the way it is at the moment I think the horizontal scroll position will always be lost whenever there is any change to the options, regardless of if the change is frozenColumn related or not, and regardless of whether the grid already has frozen columns or not (the only exception being that there is exactly one frozen column because in this case the newOptions.frozenColumn is 0). I guess my question is whether it is reasonable to make the above logic specific to only when there is a material change to the frozenColumn settings? |
Btw, it is totally fine if something like this should/could be handled in the app space, but for this one the |
the line of code you are referring to came in PR #901 to address a bug mentioned in it. I think I've put
no that's incorrect, the condition will only kick in when new options include the
as mentioned above, this was introduced in PR #901 to fix a bug mentioned in the PR, there are real reason behind the fact that it scrolls back to the top/left position.
that is what it does, this condition only kicks in when the to me the code is totally valid, For the reason as to why we have to scroll back to the origin (top/left), it seems that SlickGrid has problem to do proper calculation of header/cell positions when toggling frozen column, the best way I've found to avoid this problem was to scroll back to the origin and then SlickGrid calculates positions correctly. |
Hi @ghiscoding, As you mentioned, we can handle the issue of losing scroll position at the app level by passing delta newOptions. However, we found that the header getting misaligned when pinning the first column is a more severe issue, so I submitted the PR. Reproduction:
|
closed by PR #1039, I'll push a new release next weekend (you can remind me if I forget) |
Describe the bug
I encountered two issues:
Based on the observed behavior, I suspect the issue might be within this part of the setOptions implementation:
It seems there is an issue with the condition in the if statement. If it is changed to the following, the aforementioned issues no longer occur:
Reproduction
Steps to reproduce issue no.1:
Change
to
Which Framework are you using?
Other
Environment Info
Validations
The text was updated successfully, but these errors were encountered: