Fixes for issues with frozen columns, column reordering, and preheader visibility #591
Conversation
…ordering enabled. slick.draggablegrouping.js Only one header's ids were being accounted for after a drag and drop reorder when columns were frozen. the result is that the left (frozen) columns were fine, but the right (unfrozen) columns were 1) not sorted, and b) turned off. slick.grid.js Similar to the draggablegrouping issue, the grid was not honoring the preheader state for the unfrozen columns because only hte preheaderPanelScroller was being updated setPreHeaderPanelVisibility. For me this showed itself because I had teh preheader row toggled off and froze some columns and the headers disappeared for my unfrozen columns due to the preheader row being visible but empty and the headers being under the first row of data.
…properly use ids.header.sortable("toArray") as the array to loop on.
|
Closed and reopened because I realized I put up an incorrect version of of the scripts. Here is the first commit message that explained the causes of the issues I was seeing: slick.grid.js |
plugins/slick.draggablegrouping.js
Outdated
| // If frozen columns are used, headers has more than one entry and we need the ids from all of them. | ||
| // though there is only really a left and right header, this will work even if that should change. | ||
| if($headers.length > 1) { | ||
| for(var ctr=1,l=$headers.length; ctr < l; ctr+=1) { |
There was a problem hiding this comment.
what does ctr stand for? Could you provide a better and perhaps longer name to make it more explicit?
There was a problem hiding this comment.
I just changed ctr and ctr2 to headerI and headerI2
Hopefully the I on the end of headerI and headerI2 are ok given the for loop below the headers.length if uses var i=0 ...
I was trying to avoid redeclaring i as a variable.
For general information: ctr is an abbreviation for counter
|
@arthur-clifford Since I know you're also using Angular-Slickgrid, have you tried there as well? Will that help in removing some of multiple events that I had to put for re-rendering the pre-header when using column grouping header? For example, all the events shown here in Angular-Slickgrid |
|
My fixes were applied in an an evnironment using angular-slickgrid. I made the changes in the core slickgrid because I was assuming you were wrapping around it for angular-slickgrid, I know that these changes work in that context. As to removing events... these changes don't necessarily affect the grouping row .. yet ... I may have another change/suggestion after I play with things a bit that may negate the need for the preheader replication for grouping. It is my opinion, and I'm open to being wrong about it, that the grouping row should be a pre-preheader outside the scrollers. I'll be looking into this stuff tonight probably. I know for my situation some data being put in the grid has a bazillion columns and having the grouping row above the scroller meant the grouping didn't scroll horizontally with the data which was desirable. I think frozen column tech moves the grouping row back into the scrollers;, or at least the left scroller. I will be diagnosing what is happening with frozen rows and grouping specifically tonight. I haven't done a lot with pull requests so I'm not sure what the preferred process is. But if you are open to the notion of a grouping row outside the scrollers I can do a separate pull request when I figure it out for my environment. |
|
There's always a first time for creating PRs, there's no good or wrong approach, as long as the code is expressive, I'm just not a fan of code that is too much abbreviated, nowadays we have all kind of tools that will do just that to minify/uglyfy the code to make it small, so I'm all in for variable that mean something and also sometime comments to understand what the next few lines does. As for the frozen feature, I was the one who did most of the code merging (and that was a lot of work) into this fork, but I'm not the one who wrote the feature, that comes from the X-SlickGrid fork. We certainly welcome enhancements when it makes sense as long as they're not breaking changes. Most of the changes should working in both Angular-Slickgrid and Slickgrid but just remember that this fork has couple thousands users while Angular-Slickgrid is just a few hundreds. I've added about 100 Cypress E2E tests here but probably not around Grouping, there's couple of Grouping Examples here, maybe just take a quick look and make sure they run and that should be enough for testing changes (most Angular-Slickgrid Examples were copied over from SlickGrid) If you can also provide print screen and/or animated gif that'd be great, I'm more of a visual person and that helps to show others what you're trying to change (I use an open source app Share-X on Windows for creating animated gif, there's plenty of apps that will work too) |
…om the ids variable rather than header variable. I also renamed headerI2 to idI and l2 to idL for the index and length variables in the for loop.
|
Just a quick FYI. I've been coding for 20+ years but haven't done a lot with PRs on a project like this. I appreciate the need for non-breaking and/or backward compatibility and basically only providing things that enhance optionally and don't get in people's way. The reason why I'm finally contributing to a project like this is because I like what you are doing, in angular-slickgrid too and I want everything to work well for everybody. I'm also more of a visual person so here is a visual guide to what this PR is about: Before all fixes in this PR, a grid with a bazillion columns Setting an anchor at snum (i.e. freezing first 7 columns) After changes made to slick.grid Side Note: you may notice that the label for the first header in the unfrozen rows is shorter than the rest, I've mucked with css in my environment enough that I don't know if that is my fault or not; either way it is not addressed in this PR. Dragging an unfrozen column before drop (without changes to slick.grid this isn't possible) After drop: After fixes in slick.draggablegrouping As to the fixes in draggablegrouping, its basically just a for loop on the $headers variable that starts its index at 1 because we already have the ids from the first header, so it then appends ids from any remaining headers so that the result is all the column ids and not just the first header's column ids which was causing the right-columns to disappear. As to grouping row (Not addressed in this PR but perhaps in future one): Just doing some in-browser html and css hacking the result might look similar to: |
|
oh nice you're talking about a lot of fixes, that's cool and yes seeing helps a lot understanding the issues. Does the dragging work with a frozen grid? I didn't think that was possible with/without fixes in the lib, which I didn't have time to investigate, I know it doesn't work correctly with grouping and I wonder if you had found a way to fix that too? They are essentially 2 div containers (4 when you also use vertical freezing) as you problem saw. Were you done with this PR? it looks ok to me after seeing what it does. |
|
I am done with this PR.
I have a potential optional possibility for the grouping row (dropbox). I’ll do as another PR.
I think I had grouping and frozen rows working together but I’m working toward all this stuff working together. Whatever I work out I’ll pass along.
And I have more to share as time permits.
…Sent from my iPhone
On Mar 29, 2021, at 5:12 AM, Ghislain B. ***@***.***> wrote:
oh nice you're talking about a lot of fixes, that's cool and yes seeing helps a lot understanding the issues. Does the dragging work with a frozen grid? I didn't think that was possible with/without fixes in the lib, which I didn't have time to investigate, I know it doesn't work correctly with grouping and I wonder if you had found a way to fix that too? They are essentially 2 div containers (4 when you also use vertical freezing) as you problem saw.
Were you done with this PR? it looks ok to me after seeing what it does.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
With a frozen grid I can drag within the frozen columns. Or within the unfrozen. But not between them.
I haven’t deep dives that but I presume it’s because only the left panels headers are being targeted.
…Sent from my iPhone
On Mar 29, 2021, at 5:12 AM, Ghislain B. ***@***.***> wrote:
oh nice you're talking about a lot of fixes, that's cool and yes seeing helps a lot understanding the issues. Does the dragging work with a frozen grid? I didn't think that was possible with/without fixes in the lib, which I didn't have time to investigate, I know it doesn't work correctly with grouping and I wonder if you had found a way to fix that too? They are essentially 2 div containers (4 when you also use vertical freezing) as you problem saw.
Were you done with this PR? it looks ok to me after seeing what it does.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
@arthur-clifford thanks for the contributions, we'll merge this one and let us know when you're done with your PRs and we can schedule a new version release. On the frozen+grouping side, there was that other issue #443 which is a UI problem with the grouping title not spreading to the right because of the containers. @6pac I'll merge this one since you ask me to review and I'm good with it, cheers |
Although I have not reported the issue, I discovered a couple issues when freezing columns while column reordering is enabled.
The commit message for the changes has details about the bugs and their causes.
These changes should be non-blocking and should improve behavior for frozen column support.