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

Inconsistencies when setting treeColumns #517

Closed
Pandabear314 opened this issue Jun 30, 2020 · 7 comments
Closed

Inconsistencies when setting treeColumns #517

Pandabear314 opened this issue Jun 30, 2020 · 7 comments

Comments

@Pandabear314
Copy link

I will preface this by saying I am not great at javascript and pretty new to SlickGrid.

When a new grid is initialized treeColumns is set directly from whatever is set in the constructions statement.
From slick.grid.js line 340:

      treeColumns = new Slick.TreeColumns(columns);
      columns = treeColumns.extractColumns();

However when using the setColumns method this is handled differently.
from slick.grid.js line 2769:

      var _treeColumns = new Slick.TreeColumns(columnDefinitions);
      if (_treeColumns.hasDepth()) {
        treeColumns = _treeColumns;
        columns = treeColumns.extractColumns();
      } else {
        columns = columnDefinitions;
      }

This can lead to odd behavior if using a simple grid but the columns may be changing.
In particular I ran into this when setting the frozenRow option, which caused my grid to not display correctly.

@ghiscoding
Copy link
Collaborator

Well looking at the code and I think it's totally normal for the second one to refer to columnDefinitions since that one is called by function setColumns(columnDefinitions), so yeah totally normal to me since you're changing the visible columns.

What is the odd behavior you're having? can you add some print screen or before & after when changing that code? But even then I think we should keep it as is, we should only provide possibility of doing a Tree View on visible columns only.

@aasdkl
Copy link
Contributor

aasdkl commented Feb 19, 2021

@ghiscoding We also found this problem when setting the frozenColumn option by:

grid.setColumns([])                     // 1. set a empty column
grid.setColumns([{....}])               // 2. set new column list (not a TreeColumns)
grid.setOptions({ frozenColumn: 1 });   // 3. add a frozen option

As @Pandabear314 said, when we call setColumns in 2nd step

Because the columnDefinitions is not a TreeColumns, the treeColumns will still be empty (grid.setColumns([]) in 1st step)

    function setColumns(columnDefinitions) {

      var _treeColumns = new Slick.TreeColumns(columnDefinitions);
      if (_treeColumns.hasDepth()) {             // _treeColumns.hasDepth() === false
        treeColumns = _treeColumns;
        columns = treeColumns.extractColumns();
      } else {
        columns = columnDefinitions;
      }

And then we do setOptions, it will set old columns into grid and cause problem

    function setOptions(args, suppressRender, suppressColumnSet) {
      // ...
      if (!suppressColumnSet) {
        setColumns(treeColumns.extractColumns());  // treeColumns.extractColumns() = []
      }
      // ...

@ghiscoding
Copy link
Collaborator

I'm a visual person, can u show with print screen or animated gif the problem?

@6pac do you have any comments on this? I don't really know what this treeColumns is

@6pac
Copy link
Owner

6pac commented Feb 20, 2021

sorry, i don't think I've ever used it. I'd have to look at the examples to see what it does.

@aasdkl
Copy link
Contributor

aasdkl commented Feb 20, 2021

@ghiscoding @6pac Sorry for reply late, Here is a codepen:

https://codepen.io/aasdkl/pen/MWbvvzx

  1. When you click 1st button, the cols will be update to new - ....

  2. And then click 2nd button to froze column, the cols will also be reset to old - ...


I found the treeColumns is a featurre added in the https://github.com/ddomingues/X-SlickGrid.

And it is introduced when introducing the forzen feature (Related: #26 (comment))

@ghiscoding
Copy link
Collaborator

hmm ok we merged the frozen feature over 2 years ago, there was so much code, didn't notice that part. Not sure if it's related at all to this Tree Collapsing Grid Example, if you have a fix that doesn't impact anything else then we would welcome a PR (Pull Request)

@ghiscoding
Copy link
Collaborator

TreeColumns got removed in #775 and released under the new v4.0.0-beta.0, so it should be safe to close this issue

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

4 participants