-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Column/Row pinning/freezing/floating - design considerations and evaluations of several forks #1033
Comments
I like your thinking on this, @GerHobbelt. I spent the past few days looking closely at, and slowly merging/rewriting, the code responsible for managing pinned columns from a fork by @JLynch7. I can see why @mleibman wouldn't want to merge this fork. It's big: includes 463 non-whitespace changes. There are changes that are not directly related to column pinning, though most are. Column pinning change necessarily touch a great deal of the code base, so it would be very difficult to make sure adding this feature didn't break others. I think a change this big should involve a rewrite at some level--jlynch built it like a bolt on, but column pinning fundamentally changes the the cache storage mechanism, display mechanics, and event handling. I'm looking at rebuilding this in as architecturally elegant a way as I can. I'm pretty set on removing pinned rows, I don't see the use case. I'm considering removing the virtualization of columns which, while it has perf benefit, doesn't have much for our use case or ones I think are likely. Also it's possible the overhead of the virtualization calculation could offset the benefit of virtualizing the very few columns that are off screen in our use cases. QuestionsCan we remove the idea of virtualizing columns/cells?There is a perf gain for virtualizing off-screen columns, but I wonder if this has real-world benefit that offsets the code complexity this introduces. I think most real-world cases aren't going to have "many" (>30) columns, only many rows. Would it be a big feature hit to remove this? reduces the complexity in some areas by 30% or more. Who needs pinned rows?There's a fixed header, to which you can add a header row, paired with columns, and a topPanel, spanning all columns. are pinned rows needed in addition to this? I don't think so. There's some use for an iphone contacts style group headers that scroll with content and glue to the top until the next one comes by, but just pinning N rows... I want to see a use case. Key ChallengesHere are some key challenges I've seen with pinning N columns left. Keeping the
|
Hi folks, I also think there needs to be a bit of an education campaign about pull requests. For various forks to be effectively maintained, it's absolutely essential that pull requests from the community be atomic and minimal, otherwise every one must be reviewed and possibly reworked. I say that because I'm also maintaining a fork that implements quite a different DataSource and has major changes to the codebase. It's great that people contribute pull requests, but disappointing when I find that there are so many different changes wrapped in a given pull request that I have to pick them all apart before they are of any practical use. Ben |
@6pac: this is not going to be an atomic or minimal pull request. In fact, I doubt I'll do a pull request. Column pinning touches so, so much--draw mechanics, yes, but also the cached dom element storage format, event binding, and on and on. You show me how to do column pinning without a massive rewrite and I'll give you an enthusiastic high-five. By implementing it with fewer features enabled, there are actually fewer codebase changes to implement. This is pretty much the fork of no return, as far as I am concerned. @mleibman appears to be abandonwaring the project anyway, and it is showing it's age (hard dependency on jQuery and support for browsers that don't matter any more). |
I agree, sorry but you're misunderstanding what I said. I am in the same position as you. I also have done a 'fork of no return'. Not a pull request, but a fork. And I clearly agree that this is necessary for some purposes. I'm fascinated that you think this is old though - I regard it as the best, most cutting edge grid available. Not that I'm really up with these things. What would you use in preference ? |
Does anyone understand what the whole StackO question: http://stackoverflow.com/questions/25172683/why-does-slickgrid-add-1000px-in-js-then-reverse-it-in-css-style |
@6pac I think it's the bestest grid ever too. But, it hasn't been touched in more than 6 months. From the maintainer:
The jquery dependency should go, as should the support for old browsers (lte IE8 or 9). Requiring a browser with |
We're wandering wildly OT here, but I like what you're saying. From what I understand, you basically want a HTML5 version of the grid. |
This renaming was the major effort today. I felt I needed it in order to make sense of all the required dom elements. Does this read ok? /*
## Visual Grid Components
To support pinned columns, we slice up the grid regions, and try to be very clear and consistent about the naming.
All UI region info objects start as an array with a left [0] and right [1] side
Dom elements are stored at the top level together (still in a left/right pair) because jquery has convenience methods for dealing with multiple elements. (eg: el.empty())
topViewport.width // combined width
topViewport[0].width // left width
topViewport.el // both els
topViewport.el[0] // left el
topViewport[0].el // left el, too? A: no. undefined. let's only have one reference.
*/
// [0] [1]
// ....................
var topViewport = [{},{}], // . . . // The scrolling region
topCanvas = [{},{}], // . . . // The full size of content (both off and on screen)
header = [{},{}], // . . . // The column headers
subHeader = [{},{}], // . . . // Optional row of cells below the column headers
// ....................
contentViewport = [{},{}], // . . . // The scrolling region for the grid rows
contentCanvas = [{},{}], // . . . // Full size of row content, both width and height
rows = [{},{}]; // . . . // Container for information about rows
// . . .
// . . .
// . . .
// . . .
// . . .
// .................... |
Yup, it does read fine. :-) |
Hi guys! |
Hi Guys! Thanks a lot to all of you!Juan Lanus |
See #1055 |
From which fork can i pull from to get the group column headers and ability to pin columns? It's great that you guys are coordinating...i feel like there should be two main forks..one such as the one 6-pac is maintaining and perhaps one that incorporates these major/big enhancement features. Hopefully after they have baked for a while it can be merged back into 6pac's or mbliebman's. |
SimpleGy is working on a pinned columns implementation. There are several other older forks that have pinned column implementations he's using as inspiration. I share his opinion that the code changes are too sweeping to be able to be re-integrated back into the main trunk. I've taken the opposite approach - trying to make it possible for essential small patches to be pushed out to divergent forks to keep them up to date. This is why I have posted the patch list. |
Following up on my msg to @SimpleAsCouldBe in #897, I think it's better to collect the considerations and work / progress info regarding this big issue in a separate issue -- and, yes, I know the issue tracker already has several issues regarding this subject, but all are specific as to a particular design or pull request.
@mleibman once wrote that he would not consider merging any of the forks that offer this type of functionality for a while as he had pondered the problem himself and concluded that none of the available solutions delivered sufficiently on the underlying issues. Regrettably I was never able to dig up his conclusive analysis and list of underlying issues. This is an occasion where I fear I've been retracing someone else's steps.
Let's answer @SimpleAscouldBe question, rephrased as: where are you guys at and what have you found out?
The text was updated successfully, but these errors were encountered: