Skip to content
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

Feature request: Frozen columns #26

Closed
at88mph opened this issue Feb 15, 2016 · 73 comments
Closed

Feature request: Frozen columns #26

at88mph opened this issue Feb 15, 2016 · 73 comments

Comments

@at88mph
Copy link

at88mph commented Feb 15, 2016

I've been using this branch of SlickGrid for the past couple of years:
https://github.com/JLynch7/SlickGrid/tree/2.0-frozenRowsAndColumns

But I would like to keep the underlying SlickGrid up to date with yours. The idea is that one can set the first n columns to be frozen and would remain in place while scrolling. Perhaps more of a spreadsheet feature, but useful when selecting rows.

Many thanks for keeping this current.

@6pac
Copy link
Owner

6pac commented Feb 16, 2016

It's always a challenge keeping things updated across multiple forks.
Hopefully my patches are fairly small and atomic, and will merge easily (that's the idea, anyway).
Let me know if there's anything I can help with. Also interested to see how you approach it (eg. a new fork for the merged version?)

That said, I'm cautious about 'officially' supporting the frozen column forks because that feature does seem to modify some of the core render code. There seem to be a few options for frozen column implementations out there. It would be good to identify one (if that's even possible!) that's stable and has a good range of features. I've called out in my Wiki for anyone to submit knowledge and experience of the different forks out there. Feel free to submit what you know.

@fortesl
Copy link

fortesl commented Jul 12, 2016

So will frozen columns be implemented in the 6pac fork? I'm also using the JLynch7 fork for the frozen columns feature. Thank you

@6pac
Copy link
Owner

6pac commented Jul 18, 2016

I think officially, no. However, I'd be happy to be a part of taking a look at the fork to see how the differences between the two forks could be minimized. This would maximize the portability of future updates to my repo.

@CSTufts
Copy link

CSTufts commented Oct 6, 2016

+1

@mpetkov
Copy link

mpetkov commented Oct 28, 2016

I am also switching to the JLynch7 branch beacuse I need fixedColumns. It would be great to implemet that feature in this repo which is up to date.

@6pac
Copy link
Owner

6pac commented Oct 29, 2016

been on the 'to do' list for a while. i'm just too busy.
if anyone is keen to do the main parts of the merge, i'd be happy to test and troubleshoot to get it to standard. my main problem is actually disentangling out what the changes are for that feature.

@at88mph
Copy link
Author

at88mph commented Oct 29, 2016

No kidding. I tried it once and the frozen code from that branch is interwoven quite nicely. Perhaps a new implementation is in order?

Thanks for the replies, Ben.

@6pac
Copy link
Owner

6pac commented Oct 30, 2016

In some ways I'm thinking rather than try to integrate JLynch7's changes to my repo, it might be a lot easier to just fork JLynch7's repo and apply my changes to it.
Would also depend on at what point JLynch7 forked from MLeibman and how well it was kept up to date after that. Any intel on that, anyone?

@chhunchha
Copy link

No intel on that 6pac, but I have been using that fork for frozen column which is very critical feature for me. I worry that in future might have to switch to some other alternative as it is behind in jquery upgrade also.

@6pac
Copy link
Owner

6pac commented Nov 30, 2017

The sad fact is that SlickGrid needs months of work put into it to integrate a lot of the branched code, including the post-jQuery branch, etc. I will never have time to do that.

The only hope is if it is adopted by a project that is generating cash and considers it critical enough to put some effort into. I have a project that might fill that role, but too early to tell. I keep getting distracted by having to make a living!

@karmis
Copy link

karmis commented Dec 6, 2017

@6pac its very sad =( ; Frozen columns is very necessary feature;
+1 for it

@6pac
Copy link
Owner

6pac commented Dec 11, 2017

OK, so I was on a long plane flight, and downloaded the JLynch repo before I left. I have found that the changes are not really that extensive, although they do complicate the rendering code a lot. Also, the Jlynch branch has not been maintained for about 4 years.

Using WinMerge (an awesome tool) I have been able to reformat the my current and the JLynch slick.grid.js to highlight the differences. It's probably only a day's work to go through and copy the relevant bits over.

slick-jlynch-compare

Is anyone interested in helping with this task?

It's also worth a conversation about exactly what frozen columns means.

@at88mph
Copy link
Author

at88mph commented Dec 18, 2017

The JLynch design as I understood it was to create two separate, scrollable viewports, where the viewport on the left represented the n number of columns to lock. We could just port over the code from JLynch, but what if there were a better way to do it now? It could warrant some investigation, especially since much of the code is in conflict with the current @6pac master.

As for what's expected from it, I see it working like this animation. The identifier or row selection mechanism is in the first column typically, unless someone has a different use case, so optionally locking the first column would be sufficient, I think.

Thoughts?

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 18, 2017

I would say that the locking of first column is a good start, but in some cases I also like to use the last column frozen as well (for example I would put the edit icon in 1st column but the delete icon in last column, so that they never interfere with each other).

EDIT
I was using UI-Grid with AngularJS and they have a demo that you can take a look at. They call it Pinning instead of column freeze but it's the same concept. They allow to pin any number of columns at the beginning (1 or more) and at the end (1 or more) but never in the middle because it's way too complex and there's no usage for that anyway. So take a look at their demo

@6pac
Copy link
Owner

6pac commented Dec 19, 2017

Well, making three viewports would be not much more difficult than two!
I was thinking it might be a good idea to use if statements to keep the current single-viewport code as-is and just use the multi-viewport sections for when the feature is switched on.
While it would duplicate code, it's often useful to have the simpler version available as an easy-to-understand template, and then just mirror any changes to the more complex version. It's a tricky line to tread.

In this case, I think the additional advantage of the much simpler HTML, leading to presumably significantly less work for javascript and the browser, makes this approach compelling.

@6pac
Copy link
Owner

6pac commented Dec 19, 2017

Had a look for recent HTML/CSS solutions to this problem, drew a blank :-(

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 19, 2017

If I look at the UI-Grid, it looks like concept wise it's actually 3 viewports (left, center, right) which are set with an overflow horizontal (no vertical). These 3 viewports are inside a bigger container which has an overflow vertical so that it controls the 3 viewports while scrolling up/down. You can pin 1 or more column inside each viewport (with this pinnedLeft: true, which you can apply to multiple columns of the left and the same goes for pinnedRight:true).

They also seem to calculate margin left and right to center the middle viewport. I guess we could do it too since we know the width of each columns. Consider that your full container has 1000px wide, and you pinned 2 on the left and none on the right and that your columns are 100px wide, that you mean that the middle viewport has a margin-left: 200px and margin-right: 0px. That part is done with jQuery.

That is just what I seem to understand from the UI-Grid html code, is that even doable with SlickGrid, I seriously don't know but it's a start.

@kjn2
Copy link

kjn2 commented Mar 16, 2018

Hi, I am not a coding expert, but from what I learned about slickgrid, my 5cents of input would be to not compromise the performance of this branch by solving the problem in html/css vs. the underlying canvas technology. We're planning on using slickgrid primarily because of it's awesome performance and memory footprint. Let's not compromise that just to get a feature implemented quick&dirty so to speak... and again, not implying anybody is trying to do that, only 'product development' feedback.

Somebody was asking for some help in coding... I possibly have someone who could help out a bit. Clearly, the person needs some intro and guidance, but is familiar with Canvas and js and others.

@6pac
Copy link
Owner

6pac commented Mar 17, 2018

Yep, thanks, @kjn2. The pinned column functionality is ready to be integrated, and in this case I don't think there even is a 'quick and dirty' option. We just don't have anyone at present who has the 10 hours or so required to transfer the feature from the (quite outdated) @jlynch repository to this one. I don't think it would be too hard, with the right tools, but it's just a time issue.

@kjn2
Copy link

kjn2 commented Mar 17, 2018 via email

@atifsyedali
Copy link

atifsyedali commented Apr 5, 2018

@6pac Any updates? Would love to have the pinned column feature, even if it is only for one leftmost column.

There was some discussion earlier about how the pinned column should interact in the presence of row grouping. Have you seen airtable.com? It basically combines the row grouping portion with the pinned column viewport.

image

You can also make ag-grid behave somewhat similarly. For example, try the following with their demo: https://www.ag-grid.com/example.php

image

@steve192
Copy link

steve192 commented Apr 20, 2018

I am also very intrested in this frozen columns feature. I am in a project where we need to render huge tables with the functionality of fixed columns (~ 1000 lines and ~1000 columns ). SlickGrid is one of the only frameworks we could use, that have the performance to render that many data without lagging and still have a lot a features for a nice display of data. So I can invest some time in the project.

Sadly I am not a very experienced javascript developer regarding rendering, canvas and such and have no overview of the internals of slick grid. Do you think I can help you anyway ?

By the way, JLynchs implementation still has a bug:
If you scroll all the way to the right and scroll down a bit, the content of the fixed columns is not rendered anymore, even though they are visible.

@chhunchha
Copy link

chhunchha commented Apr 20, 2018

@steve192 I forked JLynchs - 2.0-frozenRowsAndColumns branch and did some bug fixes and it works perfectly. Wasn't that hard. Let me know if I can help anyway

@bellma-lilly
Copy link

@steve192 @chhunchha I think you are referring to JLynch7/SlickGrid#56
I have a bug fix for this and some other issues as well, should JLynch's code be integrated here.

@6pac
Copy link
Owner

6pac commented Apr 25, 2018

OK, so the kind of changes I'm thinking of are:

  1. start with the JLynch repo and the modified current slickgrid (from which I've removed all the changes not relevant to the pinned columns feature)

  2. Add additional variables for the extra viewports etc. A sample from JLynch:

     var $headerScrollerL;
     var $headerScrollerR;
     var $headerL;
     var $headerR;
    
     var $viewportTopL;
     var $viewportTopR;
     var $viewportBottomL;
     var $viewportBottomR;
    
     var $canvasTopL;
     var $canvasTopR;
     var $canvasBottomL;
     var $canvasBottomR;
    

I would leave the original object and name the extras accordingly, eg. $viewport, $viewportR, $viewportL Note JLynch has four objects because he supports top row and left side column splitting. If we were to support left and right sided column and top and bottom row splitting, we would need nine!

  1. Go through the code diff between the two files (see screenshot above), keeping the existing code but adding the new code, eg:

     if (pinnedFeatureIsActive) {
       //existing code
     } else {
       // JLynch code
     }
    

This has the following advantages:

  • it would allow the original code to be kept stable
  • it would mean any performance issues introduced by the more complex rendering would be able to be limited only to the cases where it is used
  • it would also preserve it as a simpler example of the core render code free of the pinned feature complications.

There have been some good offers, particularly from @kjn2.
The thing that worries me is that this is a bit like brain surgery. It's straightforward enough if you can copy and paste from one side to the other while understanding and investigating the implications of each decision. But if you can't walk that tightrope, it's possible that the end code might take more time to fix than to start again.
I don't think it's necessary to have great familiarity with the code, but it would be necessary to have an advanced understanding of javascript.

As I have said, I personally don't need this feature and can't justify the time to do it right now. Especially given that it will probably lead to further complications, as illustrated by @atifsyedali

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 15, 2018

Alright everyone, I decided to take the 10 hours needed and I use a tool to compare (ExamDiff), it was painful and there's a LOT of code change. There will be also a lot to review and possibly some changes here and there... However I'm happy to say that... it works!!!

The source I used X-Slickgrid
The branch you can test it all out is feature/frozen-grid

Why X-Slickgrid and not JLynch/frozenGrid? From my understanding X-Slickgrid was a fork of JLynch - frozenRowsAndColumns and is more recent (last commit 2016 vs 2013), which is why I based myself on X-Slickgrid.

instructions / notes

  • you can test all the files that start with the filenames "example-frozen", there's 7 in total
  • I'm not sure if it works with jQuery 3.x, I updated all the examples to use the same version as the current slickgrid, which is jquery-1.11.2.min.js (same goes for jQuery UI)
    • it might fail with jQuery 3.x, I see there's a few jQuery call that are different and they will have to be reviewed. There's a lot of DOM element call with elm[0] instead of elm in current SlickGrid version, is that related to jQuery 3.x? I don't know, maybe... if you know please shed some light
  • feedback are welcome, however I am not the author of the code per say. I have done my best to merge (over 2 days) as much as possible but someone (most probably a few peoples) will have to review it all out

Final Note

  • The X-Slickgrid is not only column freeze, it also support row freeze. You can see the multiple examples provided
    • column freeze seems to always start from the left but row freeze is more flexible
  • Please don't ask when that will be merged to master. If you want to make it happen, then please provide feedback, test it out and help us make it happen.
  • There is a possibility that it will never get merged... hopefully not but I'm saying this because it's probably the biggest PR touching the core grid.js file in current SlickGrid fork.
  • This is the last biggest feature requested in this fork, let's make it happen!

You're welcome ;)

@ingodjango
Copy link

ingodjango commented Dec 2, 2018

I have tested all examples and most behaved as they did with the examples before.
My Observations:
a) example9 row reordering: pulling an entry to the recycle bin is not working. This is not because of this branch it's also not working in slickgrid 2.3.2
b) example-frozen-columns-and-rows-spreadsheet behaviour is strange when frozen columns and rows are enabled. Selecting shows issues as well as copying more than one cell from one quadrant to the other. Copying single cells work. Selecting with the mouse in the upper left quadrant also worked, but the selection frame won't stop at the border to the other viewport. The issue is there in this branch. The Selection worked in JLynchs implementation (I guess the plugin for the cellrangeseletor didn't made it into the branch). But copy and paste was with JLynch also only pasting one cell and not more, there the example should throw an error that only single cell copying is possible but lenghts that are coming back are wrong.

The "normal" example-excel-compatible-spreadsheet works as before.

HTH

@ingodjango
Copy link

I also found the issues with example-frozen-columns-and-rows-spreadsheet:
the slick.cellrangeselector.js plugin needs to get merged with the one from JLynch, second the decorator get's the wrong canvas and is therefore attached to the wrong canvas.
In slick.cellrangedecorator.js line 44 needs to get changed from:
.appendTo(grid.getCanvasNode());
to
.appendTo(grid.getActiveCanvasNode());

HTH
BR
Ingo

@ghiscoding
Copy link
Collaborator

@ingodjango
Are you refering to this slick.cellrangeselector.js from JLynch? There seems to be a few line of changes between the 2 files. It would help if you could provide file link when necessary, to validate that I'm looking at the correct file(s). Thanks

@ingodjango
Copy link

@ghiscoding: okay will do.

  1. Yes, this one: https://github.com/JLynch7/SlickGrid/blob/2.0-frozenRowsAndColumns/plugins/slick.cellrangeselector.js needes to get merged

  2. And this one: https://github.com/6pac/SlickGrid/blob/feature/frozen-grid/plugins/slick.cellrangedecorator.js
    line 44 needs to get changed from:
    .appendTo(grid.getCanvasNode());
    to
    .appendTo(grid.getActiveCanvasNode());

@ghiscoding
Copy link
Collaborator

@ingodjango I tried as much as I understand to merge the 2 slick.cellrangeselector.js files, but I'm not sure if this is how you want it. I also don't know what needs to be tested, so can you go ahead with whatever test you need to do. Let me know if there's any line to change and provide them if need be. Thanks

@ingodjango
Copy link

Thanks @ghiscoding it works. I also test all the other examples again and it's works pretty fine.
Also in the context of my own usages of the grid/this Branch works without any issues so far.

@ghiscoding
Copy link
Collaborator

@ingodjango
Another thing I had to do was to retest the new code with my repos (Angular-Slickgrid or Aurelia-Slickgrid) which are just wrappers of SlickGrid for these frameworks and are coded in TypeScript. Only 1 problem surfaced the code you provided, a catch is a method and it must have an argument to be valid. So I just changed try { } catch { } to try { } catch (e) { } and that became valid and finally tested all my repo examples and they are all working, great!

The final step would be to get the code in sync with all the commits that were pushed since I created the branch. The last sync seems to be May 18, so I have a few commits to be up to date but that shouldn't be too bad. I'm not sure when I'll have time to do that but it will happen before xmas for sure.

Final note... it's looking good 👍

@ingodjango
Copy link

@ghiscoding: Thanks for the update. I'm happy that it works, with the correction.
So if you need some help please let me know.
Best Regards
Ingo

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 20, 2018

@ingodjango
Alright I went trough all the commits that were missing. As much as I could, I cherry-pick the commits and when that was not possible, I made the changes manually with same commit message and reference the commits inside all the commit message.

So all that to say, we are now in sync with all latest commits (I counted 35 commits in total).
Some of these commits might affect our changes, mainly the commits done in the slick.grid.js file.

We need to do a final round of tests @ingodjango could you redo all the tests?
I will do the tests in my repos (wrapper of slickgrid core).

Thanks

EDIT

Looks all good in my repos

@ingodjango
Copy link

@ghiscoding: Yes, will do the test within the next days.
BR
Ingo

@ingodjango
Copy link

ingodjango commented Dec 21, 2018

@ghiscoding
Here are my findings
@6pac There are also two issue that I have seen as well in the master branch

example16-row-detail.html:
This following issue is also in the master branch, so it got intorduced to this one.
When setting "Set Max Panel Rows Option" and open the row with a click a couple of empty rows are added below the clicked row.

example-excel-compatible-spreadsheet.html
The cell-selection is not working since it's now only possible to select one cell, and not multiple by dragging

example-draggable-grouping
When clicking Toggle Draggable Grouping Button the column header moves up and shows a whitespace row at the top underneath the header.
In the master Branch there is more or less the same issue except that the whitespace ro appears at the bottom.

example-explicit-initialization.html
The header-row is broken and the rows are not in the container

example-spreadsheet.html
Also here the cell-selection is not working any more

example-frozen-rows.html
When Frozen Bottom Rows checkbox clicked and set then the scrollbar shows a strange behaviour when dragging the scrollbar handle

BR
Ingo

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 21, 2018

@ingodjango
Are these all new behaviors that were not there before?

If so, need to find the commit(s) that affected them. By trying to rollback the changes 1 by 1, there's couple of commits that I was not sure about, like this one and this other one

EDIT

@ingodjango
I found the problem with the row selection, that is for example-excel-compatible-spreadsheet.html and example-spreadsheet.html. The problem is since you told me to get the file slick.cellrangeselector.js from JLynch. Specifically, this line. If I put back this piece of code:

if (!_grid.canCellBeSelected(end.row, end.cell)) {
        return;
}

it fixes the non frozen grids, but if I try a frozen grid like example-frozen-columns-and-rows-spreadsheet.html, I don't think it works as intended, that is with or without the code change. If I had multiple values, and try to copy, it only copies 1 value. However if I try the same example in the X-Slickgrid repo, that same example doesn't copy anything... so should we be spending time on this?

@ingodjango
Copy link

@ghiscoding
I checked it and it seems that all issues where there before as well except that one:
"example16-row-detail.html:
This following issue is also in the master branch, so it got intorduced to this one.
When setting "Set Max Panel Rows Option" and open the row with a click a couple of empty rows are added below the clicked row."

That option wasn't there before.
So I guess a rollback is not necessary and we should look to find the issues.
I will try to dig into the stuff within the next days.
BR
Ingo

@ghiscoding
Copy link
Collaborator

@ingodjango
please read the comment I just added in the previous post. As for the Row Detail example, you can skip that one for now, I worked on those changes and I can come back to it later. I'd like to fix the other examples first

@ingodjango
Copy link

@ghiscoding
Haven't seen that the comment was edited, sorry.
So the implemtation from JLynch didn't cover the case of multiple copying. That's from the example:

    copyManager.onPasteCells.subscribe(function (e, args) {
      if (args.from.length !== 1 || args.to.length !== 1) {
        throw "This implementation only supports single range copy and paste operations";
      }

so if it only copies one value like you describe it, it behaves correctly or as intended.

The Error (trying to copy more than one value) is not catched correctly since the args.from.length doesn't return the values that would make the catching of the error correct. So it's an issue with that example and not your code/merge.
HTH
BR
Ingo

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 21, 2018

I just fixed the copy of multiple cell with/without frozen columns. So that's done

So from your list,

  • example16-row-detail.html:
    This following issue is also in the master branch, so it got intorduced to this one.
    When setting "Set Max Panel Rows Option" and open the row with a click a couple of empty rows are added below the clicked row.

  • example-excel-compatible-spreadsheet.html
    The cell-selection is not working since it's now only possible to select one cell, and not multiple by dragging

  • example-draggable-grouping
    When clicking Toggle Draggable Grouping Button the column header moves up and shows a whitespace row at the top underneath the header.
    In the master Branch there is more or less the same issue except that the whitespace ro appears at the bottom.

  • example-explicit-initialization.html
    The header-row is broken and the rows are not in the container

    • I think this problem was there before I started adding all the commits, and I believe it's because this grid is inside a <table> and we shouldn't do that
  • example-spreadsheet.html
    Also here the cell-selection is not working any more

  • example-frozen-rows.html
    When Frozen Bottom Rows checkbox clicked and set then the scrollbar shows a strange behaviour when dragging the scrollbar handle

I'm not sure what you mean with last example, I don't see anything strange. Maybe you can install an App to do animated gif of print screen and show it. I use ShareX for to do that.

EDIT

I just found the problem with the explicit initialization, 1 line of CSS position: absolute; was causing all this UI problems. That actually fixed 2 examples (explicit init & draggable grouping).

So there's only the last example, but I don't know what you mean. I don't see anything strange. Can you do another round of retest since I did some code change. I tested all examples (19 of them) in my repo and they are all good. I'll wait for your tests, so far it's looking good.

Thanks

@ingodjango
Copy link

@ghiscoding
Tested it: Awsesome!

regarding:
example-frozen-rows.html
example-frozen-rows.zip

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 21, 2018

@ingodjango
hmm ok that is a little funny indeed, I tried with X-Slickgrid and that doesn't occur. I think @6pac added some kind of calculation for the scrolling height or something. That might be hard to find the root cause though. It's not a major problem though

EDIT

I had to rollback the deletion of position: absolute of the .slick-pane. I noticed that it was there in X-Slickgrid and there's a good reason, all the frozen examples were broken without it. I fixed the explicition initialization directly in the example itself, but now the draggable grouping example is back to previous behavior. Trying to fix it

EDIT 2

I got the draggable grouping example working. The pre-header toggling was using direct jQuery, but we should never do that, instead we need to use the grid setPreHeaderPanelVisibility and now it's all good

EDIT 3

I tested ALL examples, there's only that Frozen example scrolling that is a little weird, but every other examples are all good now.

ALL Resolved

@ghiscoding
Copy link
Collaborator

ghiscoding commented Dec 22, 2018

I'm very happy to announce that everything now works and is also in sync with latest commits. I tested all SlickGrid examples (62 of them) and I also tested my repos (Angular-Slickgrid and Aurelia-Slickgrid) examples (19 of them). it's all good, it's a Go 💯

@6pac
How do you want to proceed?

At some point, I thought we could possibly support both the old and new, but then I see that this PR touches most core files, that is slick.grid.js, slick.core.js, slick.grid.css and couple of the plugins with small adjustments. Considering this, I think the only remaining option is to merge and replace previous version and bump SlickGrid version to 2.4 or 3.0. We could also rename previous file from slick.grid.js to slickgrid.js for reference, in case something goes wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests