6pac / SlickGrid

A lightning fast JavaScript grid/spreadsheet
https://stackblitz.com/github/6pac/SlickGrid/tree/master/vite-demo
MIT License
1.86k stars 425 forks source link

Feature request: Frozen columns #26

Closed at88mph closed 5 years ago

at88mph commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

+1

mpetkov commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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

6pac commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

ghiscoding commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

A friend of mine in India has lots of Canvas experience... not sure if that's required.  If you think that skill would help you, let me know...I could 'fund' a few hours of his work. 

 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.—

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

atifsyedali commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

6pac commented 6 years ago

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!

3) 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:

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 commented 6 years ago

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

Final Note

You're welcome ;)

6pac commented 6 years ago

This is awesome! I'll check it out on the weekend.
I'll do everything I can to commit this to the main repo, but you are correct, it may take a little while.

ghiscoding commented 6 years ago

To do the review, I strongly suggest to use a diff tool, I just found this tool MeId and I wish I had it when I started, this tool also highlight addition not just the entire line change (my previous one was highlight the entire line). It seems WinMerge (previously known as WinDiff I think) also does addition/removal highlight which is great.

I will do a second review myself, because there's a lot of code that was replaced through this process and looking at it with this new tool, I can see right away that I forgot some variables (see screenshot) var $preHeaderPanel, $preHeaderPanelScroller, $preHeaderPanelSpacer;

I didn't see this with my previous tool and I wish I had search for such tool prior.

grid_diff

@6pac Question for you, I can see that the $preHeaderPanel was something added only in your fork, is that taking the entire width on the top or should also be separated Left/Right like all the other variables for the frozen grid? I mean if it takes the entire width at all time, it won't be split by a frozen grid, then I will leave your code as is. I would prefer that actually, that would be 1 less thing to think about, I don't think the pre-header would split but you can confirm.

6pac commented 6 years ago

I think the $preHeaderPanel should be left alone. One other thing is worth mentioning: the if/then construct. I did give a pretty detailed strategy a few posts up. This was designed so that no core code actually changed except in an if statement.

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

I know it violates DRY, and could potentially be regarded as a mistake but I think it (a) keeps core code stable (b) possibly makes the grid faster when splitting is NOT used (c) means we have a copy of the 'simple' code to debug basic issues on, which can then be propagated to the more complex case once solved.

Of course this doesn't apply to var declarations and infrastructure, just the active code.

Excuse me if you've done this, I haven't looked yet!

ghiscoding commented 6 years ago

I just tried it with my Angular-Slickgrid which uses jQuery 3.x and it all works, except for 1 grid that uses the pre-header, which might be because of the missing variables that I wrote in my previous post.

Also worth to know, my start was not from your code but instead from the X-Slickgrid code because there was way too many to copy in your grid.js. So I took X-SlickGrid file of grid.js and started copying your grid.js into the X-SlickGrid one.

@6pac I think the best is if you go directly in the branch and you make any changes directly in there, Using the diff tool is ideal. I can then pull latest code and review it after. I will do another review before the end of the week.

Also just so you know, I did not think about the logic, I really tried to merge and re-use your code as much as possible but at some point it was too complicate and I used the X-Slickgrid code instead. I know that some of your logic got replaced, which is why we need to review and put back some of your logic in there

and we cannot keep some of your code, like viewport for example is now split in 2, viewportL and viewportR. I don't think we have a choice there, if you want to do it like you said, the code will grow exponentially.

EDIT Some observations of what is not behaving correctly

kahluagenie commented 6 years ago

This would be so cool to have! I inherited a project that is using JLynch's 2.0-frozenRowsAndColumns branch. Would love to get it on this fork instead!

ghiscoding commented 6 years ago

I have put a lot of work on this branch and mentioned what is working and not working in previous post. But nobody wanted to help neither provide feedback, so it's kind of paused right now.

6pac commented 6 years ago

Yeah, sorry I haven't been able to be more help. I'm too busy to do anything until at least September. crunch time in two big projects. I've gotta say, we headed in slightly different directions on this one. It'll take a bit to get my head around it.

psvoboda76 commented 6 years ago

Hi,

I can see a lot of work has been done to provide a frozen columns/row feature. I think the feature would be really good to have. I am willing to help, just let me know how I can. I can either help to review or write the code, do the testing. Also, I can help a bit financially if it would help?

Thanks for the feedback and have a nice day

Petr

ghiscoding commented 6 years ago

@psvoboda76 I wrote in earlier comments that I started the work on a different branch

The branch you can test it all out is feature/frozen-grid

So if you download the fork with Git and change branch to that mentioned branch, you'll get the code. There's a big portion already working, and there's also a small list of things that aren't working as expected, you can see the list also in earlier comment

ingodjango commented 6 years ago

I used Jlynchs version (it had a couple of issues) and replaced it with your feature-branch and so far works pretty well. I can confirm that hamburger is not working. @ghiscoding in regards to the observations: what could I help there? Not sure that I understand what help is needed. Hamburger is also not working on my end, with the boostrap styling I us it also breaks the layout. I use so far a different editor with custom formatter which works all fine. So what can I do/help?

BR Ingo

ghiscoding commented 6 years ago

@ingodjango What's left to do is fixing all the things that aren't working, like the Grid Menu and all other features that are not checked in the list.

This is a huge branch/PR which affects the core lib of SlickGrid, what we have to do is fix all problems but also make sure that the branch is up to date with latest changes (it's not currently). Once that is all done, we need a few volunteer to use/try it in their project... and finally when that is all done, we need the benediction of @6pac before merging.

If you have time to fix all remaining problem from the list, that would be great

6pac commented 6 years ago

@ghiscoding , unfortunately you know I won't get to helping with this soon :-(

Recently there was this post on StackOverflow. While I don't necessarily agree with the sentiment (eg. I still have to support browsers that don't support ES6, and also NPM is not necessarily the definitive development environment), you have pointed out before the the ES6 repo has done away with the custom drag and drop libraries (and jQueryUI too?), and I note that it has also integrated the pinned rows function.

I haven't researched that repo and the ES6/npm requirements at all, but this does trigger a reaction in me that perhaps we should be standardising on something like the ES6 repo, perhaps providing an ES5 downgrade, and concentrating our efforts there. As I said in the comments, I don't use NPM or NodeJs at all, but I wonder how much trouble it would be to start standardising to that, in the interests of long term compatibility?

Any thoughts?

kjn2 commented 6 years ago

My opinion, apps or users that do not use browsers with ES6 support are typically running an older browser, which is most likely also insecure. If it comes to feature/function selections and optimizing the resources here, I'd opt for the following:

ghiscoding commented 6 years ago

@6pac I also don't agree with what the person wrote on that SO answer, I believe that actually it's the inverse, SlickGrid was ahead of time compare to other libraries, people tend to forget that SlickGrid is 10 years old now. Also that SO is partially incorrect, there's a big misunderstanding between NPM and ES6 bundling, NPM in itself is just a package manager (fetch library X), it has nothing to do with ES6 and that SO answer is wrong for that reason (I added a comment in the SO because that annoyed me to see such incorrect comment in that answer).

Also note that I was originally on the Slickgrid-ES6 lib, but I decided to come back to 6pac/SlickGrid because it's more maintained, I know how it works and there's no surprises. You also need to know that Slickgrid-ES6 has 2 libs in it, it uses a fork of 6pac/lib OR you can use a fork of X-Slickgrid (for pinning), so it does not have it all in 1 lib, if you want pinning then you import the X-Slickgrid, but if you want other things available in regular slickgrid then you import the regular lib. Basically you cannot use them both at same time, you need to choose which one you want. I believe that he did indeed get rid of older jQuery lib like the jQueryUI, event drag and also dropped IE8 support. Personally, I would prefer to not use ES6 but instead go with TypeScript as it adds the typing and intellisense. On my side, what I did was to create Angular-Slickgrid & Aurelia-Slickgrid, both of which are wrappers of 6pac/SlickGrid and are written in TypeScript. All the core, features, controls, plugins are still coming from 6pac/SlickGrid and that will never change in my libs, my libs are and will remain just wrappers (I still need the core of Slickgrid to be in it's own repo, which is why I'm a big contributor of the core lib as well). If there's anything that can be done, I would say it's not to rewrite the entire lib (neither make it ES6), but instead make a TypeScript wrapper of the core lib, DataView and plugins, this way anyone could use the lib (the new way or the old way) and everyone would be happy.

@6pac so please don't stress out, you are doing a wonderful job with maintaining this repo and you are getting quite close to 1000 stars. This is a big achievement... As for the original issue, I know pinning is the biggest feature left, we will get it eventually, but I won't lose sleep over it, it will come in time.

6pac commented 6 years ago

@kjn2 I mostly write intranet database sites (ie. that aren't public) and hence the client gets to mandate the browser. Try to get an entire state government to change its policy on approved browsers (not to mention updating all its workstations)! The fact is, we often have to support what we have to support without much choice. Agree that NPM is where it's at, and definitely agree that everyone is moving away from jQuery. The question is how we support all of the necessary variants without having a lot of versions of repos.

@ghiscoding thanks, you are across this way more than I am. I'll have to educate myself on these technologies, download your repos so I understand the above.
I do feel like this is the first step towards a coherent technology roadmap for SlickGrid. If we can make something versatile enough to suit all comers, that will go a long way towards unifying effort on the grid as well. Version 3.0 has been a long time coming, but it may be that it is an opportunity to restructure a lot of things and put the codebase on a modern footing.

grgsolymosi commented 6 years ago

I have put a lot of work on this branch and mentioned what is working and not working in previous post. But nobody wanted to help neither provide feedback, so it's kind of paused right now.

@ghiscoding I just want to provide some feedback: I downloaded this branch and following the example-frozen-columns-and-rows.html ("fixed couple of issues found and get code closer to previous code") code it works gorgeously so far. Great job, thank you for your great efforts. Also thanks for @6pac for maintaining SlickGrid.

ingodjango commented 6 years ago

About the discussion: My opinion sometimes some things should be finished before moving. The sad thing is we have so many stuff halfdone that would be worth finishing but people got distracted and do something else only slightly different. So I also agree that joing forces would be fine, but I would like to concentrate here and see how it progresses.

So as my prefered chef says "first things first": I looked into the hamburger. And I think I found the issue. But since I consider myself not a Javascript Pro I'm not sure where/how to fix it.

The issue is with the two panes. There are two grid-menu-buttons added. One in slick-pane-left and one in slick-pane-right. The button in the div slick-header-left gets displayed and the one in slick-header-right not. But the click event gets attached to slick-header-right which is not visible. So my spontaneous approach (in slick.gidmenu.js) would be:

      function init(grid) {
        var gridMenuWidth = (_options.gridMenu && _options.gridMenu.menuWidth) || _defaults.menuWidth;
        var $header;
        if (grid.getOptions().frozenColumn && grid.getOptions().frozenColumn >0 )
        {
          $header = $('.' + _gridUid + ' .slick-header-right');
        }
        else
        {
          $header = $('.' + _gridUid + ' .slick-header-left');
        }

        $header.attr('style', 'width: calc(100% - ' + gridMenuWidth +'px)');

which seem to work also when I put the frozenColumn Option.

BR Ingo

ingodjango commented 6 years ago

On the JQuery Accordion: for the second grid the position gets dropped. I added postition : relative at the second grid container and it worked:

<div id="myGrid02" style="width:600px;height:300px;position:relative;"></div>
</div>

BR Ingo

ingodjango commented 6 years ago

The select2-editor examples can be fixed in the example in the Select2Editor-init: from

$input.width(args.container.clientWidth + 3);

to

$input.width(args.container[0].clientWidth + 3);

ingodjango commented 6 years ago

And the last one in slick.grid.js

line 2850: from node = cacheEntry.cellNodesByColumnIdx[columnIdx][0]; to node = cacheEntry.cellNodesByColumnIdx[columnIdx][0];

I think what happened there is that with introducing the two panes/viewports the rowNodes in the cacheEntry became an array. Before in slickgrid it was the node itself. And since that update calles a row update and uses the data in the cache it got the changed data right but didn't show the update. A "normal" updateCell did they update without the cacheEntry and that's why it worked in other cases.

HTH BR Ingo

ghiscoding commented 6 years ago

@ingodjango thanks for all the fixes, I will look into adding them soon in the branch. However your last comment, the 2 lines of code are identical (unless I'm blind).

ingodjango commented 6 years ago

sorry, it's

from

node = cacheEntry.cellNodesByColumnIdx[columnIdx];

to

node = cacheEntry.cellNodesByColumnIdx[columnIdx][0];

ghiscoding commented 6 years ago

@ingodjango I pushed all your fixes, thanks for that, if I look at the list I had here it seems that all examples are now working, good job 👍

Before being able though, we need to make sure that all commits that were pushed since the creation of this branch, are loaded. Basically, we need to be in sync and not lose anything.

Lastly, we would need to get more people to try it before we can merge and get the blessing from @6pac. Again this is a huge refactoring, we need to do lot of testing, I also need to try it with my repos Angular-Slickgrid & Aurelia-Slickgrid to make sure nothing is broken there either @6pac anything you want to add?

ingodjango commented 6 years ago

@ghiscoding: the last two I guess, show that these would be breaking changes. But I think I got another solution within slick.grid.js but need to test it first. Let you know soon. Sorry for juming to fast on a (too) simple solution.