feat: Column 'hidden' property

[6pac]

Have been progressing the addition of a column hidden property, rather than having to reassign using .setColumns() whenever visibility changes. It's turning out to be relatively simple.

There is a new example example-column-hidden.html.

@ghiscoding I kown you're going to say 'wait until after the remove jQuery PR'. Happy to do that, but I'm setting up a PR so I can see what the tests are doing. Should be relatively easy to refactor this PR once jQuery is gone.

The only real sticking point at the moment is in the gridmenu plugin, which does some hanky panky to get around hidden columns. This may no longer be necessary. Just not quite sure which way you'd prefer to jump.

[ghiscoding]

what does getColumns() returns now? If it returns all columns (visible & hidden) then that would be a breaking change which I want to avoid, I would prefer to have a new getAllColumns() or something similar. It also looks like you have a merge conflict and that your branch is missing a few of the previous commits

[6pac]

Yes, there is only the one getColumns().

The way I see it is that no existing code will have or use the .hidden properly on the column. Therefore it won't be breaking - all columns will just be visible all the time on legacy code.

I just think it's complicating things a lot having a getVisibleCols/getHiddenCols/getAllCols method.

Will work on the merge conflicts - I'm still a bit new to cross-branch applying of commits.

[ghiscoding]

The problem that I see with this change is that every users are expecting getColumns() to return only the visible columns but this PR changes this to visible + hidden columns which is a big breaking change in my mind. However this is a way to make the change without being a breaking change, we could simply add an optional argument getColumns(includeHidden?: boolean) which would not break every user while also providing the new feature which the user can opt-in by provinding getColumns(true) as argument.

For the merge conflict, I use switch back to master branch then fetch latest code and then go back to feature branch and merge master into feature branch, then your editor should show the conflicts. You should also execute npm run minify script to fix all the JS map changes that I have done in previous release

[6pac]

Hey @ghiscoding, I'm not trying to be pushy here, but we keep disagreeing on one thing, and I don't think we can move forward sensibly on this before we sort it out.

The problem that I see with this change is that every users are expecting getColumns() to return only the visible columns but this PR changes this to visible + hidden columns which is a big breaking change in my mind.

All we are doing is adding a .hidden property on the column, which, if used, will hide the column. It is still possible to hide a column by completely removing it from the columns list, like legacy code does. No legacy code will use the .hidden property, so this change won't have any effect on existing code - simply, in legacy code all columns will be shown all the time, which is the current behaviour.

So, how is this a breaking change? As far as I can see, it's simply a new non-breaking feature. If a user decides to start using the feature, then they will have to adjust their code accordingly (which will be a significant simplification).

I suppose if we re-tooled all the add-ins that 'hide' columns by removing them to use the new .hidden property instead, then it would be breaking. But we don't need to do that - the old way will still work fine. We just need to make sure that the same add ins work properly if the user is using the .hidden property.

The challenge is making the use of the new .hidden property work consistently across the grid and all the plugins and controls, but that's a new feature testing issue, not a legacy code one.

[ghiscoding]

Perhaps I misunderstood but the current contract (interface wise), since the creation of the library, is that getColumns() returns only visible columns and even if the user doesn't use the hidden property yet, you are still changing this contract by now saying getColumns() method will in the future return visible + hidden columns. I did maybe oversee that if the user isn't using the hidden property then it won't affect users... but still the interface contract since the beginning of the library is to return only the visible columns (not the hidden columns) and I would prefer to keep it that way.

To not break the interface contract of only returning visible columns by default, you could simply add an argument as I mentioned earlier and that will not modify the old contract since we would only add new things via the new argument and that will not entirely change the return output

function getMethods(includeHidden) {
  if (includeHidden) {
    return columns; // will return visible + hidden
  }

  // return only visible columns
  // this would keep the legacy interface contract
  return columns.filter(c => !c.hidden);
}

I'm not sure if this is the correct code since columns might not include hidden columns (I didn't review your entire PR and I'm not sure what will happen but still I would prefer this approach since the interface contract is kept

[6pac]

OK, this is good - now we're getting down to it.

Perhaps I misunderstood but the current contract (interface wise), since the creation of the library, is that getColumns() returns only visible columns and even if the user doesn't use the hidden property yet, you are still changing this contract by now saying getColumns() method will in the future return visible + hidden columns. I did maybe oversee that if the user isn't using the hidden property then it won't affect users... but still the interface contract since the beginning of the library is to return only the visible columns (not the hidden columns) and I would prefer to keep it that way.

As far as I understand it, at the moment the library has no concept of a hidden column. There are only columns, and all columns are displayed. It is incorrect to say that getColumns() returns only visible columns, because there is no way it could return hidden columns. It simply return all columns.
This is my interpretation anyway - there is no contract to return visible columns, because there is no concept of invisible columns. And that is why we need the workaround if we want 'hidden' columns - to maintain a full columns list externally, and have external logic to pass the 'visible' columns to the grid - which is quite cumbersome and over-complex.

To me, it makes sense to add the concept of a 'hidden' column to the grid. If we do so, then we can pass a full set of columns into the grid and just toggle the .hidden property rather than maintain an external list.
Because the .hidden property is not a concept in the contract of the grid at the moment, it will be ignored by all current code and it will be as if it did not exist.

The task in adding the property is to make sure that if it *is * used, then it works properly with all current grid features. This will enable developers to get rid of the existing external 'hidden column' logic. We can keep the external logic in some plugins etc if necessary, but ideally we would offer two versions, one with hidden column support and one without.

[ghiscoding]

Sadly, It looks like you still don't want to use my suggestion though and if you don't then I still think you are breaking the contract, if yesterday the method returns only visible columns and tomorrow it returns visible + hidden then Yes to me that is a break in the contract because the output is very different. If you do make the method return visible + hidden columns, then we'll have to refactor the Column Picker, Grid Menu and maybe other plugins and we'll need to advise users of the breaking change... but if you instead apply my suggestion to your change, then we wouldn't to refactor at all and it wouldn't be a breaking change. So I'm wondering, why do you still want to go without my suggestion? I always try to avoid breaking change when I can and we could do it in this situation. I think your PR would be totally fine with my small 1 extra argument and this would having me do a lot of refactoring in my libs (I plan to refactor in the future but with my suggestion, I wouldn't have to do it right away, I could wait and do it whenever I please, it would also not break users)... basically if you intend to ship your code as it is, it has to be shipped under a major (aka breaking) version, but with my small suggestion you wouldn't have to

[6pac]

Hi @ghiscoding, sorry for the long pause, I've just been busy.

Let's be clear, I still don't agree that there's a contract to return visible columns - it should return all columns - that's exactly what it has aways done. To me, making the change you suggested is breaking that contract, and it just feels really weird to me.

Nor do I think that it's a breaking change to existing code. As always, I'm more than happy to be proven wrong. Can you provide an example where it would be breaking?

Anyway, you're clearly quite determined about this, and I do understand that you would have to do some refactoring on your libraries - that is something that I accept, so I'll have more of a think about your suggestions on the weekend when I have a bit more brain space.

[ghiscoding]

Perhaps I didn't explain myself properly, when I say contract I mean "what we have today" and when I say your change will break the contract I mean "what we'll have tomorrow". So today it returns only visible columns and tomorrow you want it to return all columns, so to me this is a breaking change to the user and it cannot be under v3.x, it will have to be under v4.x. For example, I am 100% sure that this PR would change how my libs works and I will for sure have to do some changes to my libs, because again to me, this is a breaking change since the result would be different.

My proposed change was to avoid the breaking change but still provide a way to return all columns as you wish to do with this PR.

[6pac]

LOL. This is taking up waaaay too much conversation space.

So you've said

The problem that I see with this change is that every users are expecting getColumns() to return only the visible columns but this PR changes this to visible + hidden columns

Perhaps I misunderstood but the current contract (interface wise), since the creation of the library, is that getColumns() returns only visible columns and even if the user doesn't use the hidden property yet, you are still changing this contract by now saying getColumns() method will in the future return visible + hidden columns.

Sadly, It looks like you still don't want to use my suggestion though and if you don't then I still think you are breaking the contract, if yesterday the method returns only visible columns and tomorrow it returns visible + hidden then Yes to me that is a break in the contract

I do understand exactly what you're saying, and I do understand what a contract is, but I don't agree with what you're saying, and I don't think you're hearing me.

In my mind, the contract is that getColumns() simply does what the name says, and returns the columns. All of them.

I don't understand why we would have a contract to return only visible columns.
It is true that up until now, 'all columns' and 'visible columns' have been the same thing, so this is a question that has never needed to be asked before. But I see no evidence anywhere in the codebase that getColumns() should return just visible columns. It may seem logically intuituve to you, but the exact opposite seems logically intuitive to me.

So then. If I were being dramatic I could say that from my point of view, your proposal is changing the contract and if we release it we would have to increment the major version number. I'm not trying to push that point but I'm saying all your assertions about it being a breaking change follow from your assertion about the nature of the contract.

But if you take my position for a moment, you'd see that in that case, since the users don't have the concept of isHidden yet, how could it be breaking? If they don't use isHidden, literally nothing changes.
A breaking change means a breaking change to the users, not to us. If it's breaking just for us, that's not actually a breaking change, that's just a feature release that's not finished yet! Our job is to make sure it's non-breaking within our release.

OK, enough philosophising. What I'd really like to do is add a new method getVisibleColumns() that does what it says.
Then you can just search and replace your codebase and use that method instead. When you're ready to update to ustilise the isHidden flag, you can switch back to getColumns() and make the changes.

I think this makes the behaviour a lot clearer. Adding a flag to getColumns() is to me a 'code smell' that is not technically wrong, but IMO is unintuituve and I suspect will come back to bite us in the long run.

Anyway, you are at this point the main maintainer of the project so if you are still determined, then I can make the concession.
I would emphasise that I feel the most important thing is not that I prove myself right, but that we understand each other's viewpoints and agree on a way forward. I am happy to not get my way, but only if I feel you have fully understood and considered my position.

[ghiscoding]

What I wanted to highlight by saying that it doesn't follow the contract is basically that your change is a breaking change, that's mainly all I wanted to say. If you want to go ahead by adding getVisibleColumns() method, I'm ok with that but it has to be under version 4.0.0 because of the breaking change. In other words, I'm ok with this change but only under 4.0

As for the removal of jQuery, I started testing it with my libs, I found some small issues that I provided fixes. I'll see if the user can make the change, if not, we can maybe merge it anyway since it's on the next branch and I can later apply the fixes. I have still have some small bugs to investigate but so far it doesn't look too bad and we can maybe proceed with this soon enough.

[ghiscoding]

@6pac would it be possible to rebase your PR against the next branch? I already started working on the full roadmap to remove jQuery (see this Roadmap v4 Discussion). It will require a few PRs to cover everything but its progressing well since I already migrated all Controls & Plugins to vanilla JS in my own lib and I'm expecting this work to take 2-3 weeks, so it would be good to include your PR as well. Thanks

[6pac]

OK, @ghiscoding, two things.

1. A heads up

I just noticed that in this first part of setupColumnResize

 function setupColumnResize() {
    if (typeof Slick.Resizable === "undefined") {
      throw new Error('Slick.Resizable is undefined, make sure to import "slick.interactions.js"');
    }

    var j, k, c, pageX, minPageX, maxPageX, firstResizable, lastResizable;
    var frozenLeftColMaxWidth = 0;

    const children = getHeaderChildren();
    for (let i = 0; i < children.length; i++) {
      const child = children[i];
      const handles = child.querySelectorAll(".slick-resizable-handle");
      handles.forEach(function (handle) {
        handle.remove();
      });

      if (i >= columns.length || columns[i].hidden) {
        return;
      }

      if (columns[i].resizable) {
        if (firstResizable === undefined) {
          firstResizable = i;
        }
        lastResizable = i;
      }
    }

    if (firstResizable === undefined) {
      return;
    }

The code is bailing from the loop using

      if (i >= columns.length || columns[i].hidden) {
        return;
      }

which worked when the loop was jQuery

columnElements.each(function (i, colElm) {

but should be continue now rather then return.
This appears to be in quite a few places, so I just thought I'd mention it. I might give the next branch a once-over on the weekend to check for this particular error.

2. I've been reapplying this PR manually, since it had the 'removal of the TreeColumns' PR dependent on it (my mistake) which complicated things somewhat. The removal of the TreeColumns is very modular and should have zero impact. Would you loke me to redo that too as a separate PR?

I will post the new 'hidden-columns-next' branch as a new PR soon, once the iteration issue is taken care of.

[ghiscoding]

but should be continue now rather then return.
This appears to be in quite a few places, so I just thought I'd mention it. I might give the next branch a once-over on the weekend to check for this particular error.

oh right, good catch, I assume that you are right by using continue, it's probably hard to catch with the E2E tests, so yeah a revisit of them all would be better.

2. I've been reapplying this PR manually, since it had the 'removal of the TreeColumns' PR dependent on it (my mistake) which complicated things somewhat. The removal of the TreeColumns is very modular and should have zero impact. Would you loke me to redo that too as a separate PR?

Yeah I prefer separate PRs since they are separate subjects, you might be better of closing this PR and create a new one against next to start clean. Also remember that when you tried to remove Tree on the first time, all the Frozen grids were failing. So I guess this Tree was added specifically for the Frozen Grids within X-SlickGrid

On my side, the migration is going quicker than I expected and I think I'll be done by next week. It helps that I have already done the migration in my own libs of all Controls & Plugins, I just have to compare and copy over the code that changes. So if you have time to make this PR, that'd be great and we could go with a Beta pretty soon.

Cheers

[ghiscoding]

@6pac so are you having any progress on this hidden column feature? Or should we ship a 4.0 Beta without this feature? I'm simply asking because I expect to be done this week with all of things that I wanted to address before releasing anything. If you're too busy, I would be happy with a Beta release (I'll let you know when) so that I can finish the implementation and testing in my other libs

Cheers

[6pac]

Will have it ready by tomorrow morning (my time! GMT+9:30). Just got sidetracked with the other fixes. Will also resubmit the tree columns removal PR, I'm pretty certain the frozen grids were failing tests last time because of unrelated bugs. the TreeColumn removal should have zero impact, other than losing about 200 lines of unused code.

[6pac]

This has now been superseded by #765