Grid.prototype.refreshCanvas refresh issue (ColumnFilters not showing up - height calculation issue)

[neiz]

After explicitly turning 'enableFiltering' on and not seeing any result in my grid, I began stepping through the code to see what was occurring.

Turns out (may be related to the time it takes to fetch my data for binding to gridOptions.data? I am also manually building out the column definitions) that the header heights were always calculated without the additional height to compensate for the filter.

    Grid.prototype.refreshCanvas = function(buildStyles) {
     if (containerHeadersToRecalc.length > 0) {
       // Putting in a timeout as it's not calculating after the grid element is rendered and filled out
       $timeout(function() { 
       ........
       });
      }
     };

This timeout was the culprit. By supplying a delay of 200ms, everything renders as expected. Is this a code smell of something I am doing that is not best practice? Or is there a way to correct this? I haven't analyzed this functionality yet, but am just looking for some high level explanation.

[PaulL1]

We often use a timeout 0ms to push things to the end of the digest cycle, and whilst that sort of has a smell about it, it seems accepted behaviour in the angular world. We shouldn't really ever be using a timeout with a value, because that means we're not going to the end of the digest cycle, we're waiting for something to happen and that something is potentially going to take different lengths of time on different browsers.

What version are you on? The header height issue with filters I think was addressed in rc20, or maybe in unstable. And if you're turning on the filters after the grid renders, then you also need to call notifyDataChange I think in order to trigger the height change - the tutorial provides an example of that.

[PaulL1]

In the filter tutorial this works: http://ui-grid.info/docs/#/tutorial/103_filtering

I did previously had a problem where it only worked if the filters were initially enabled and then disabled, but not if they were disabled and then later enabled. Creating a plunker for that: http://plnkr.co/edit/0osD3ckxLXmA64dayHQR?p=preview it seems to work fine now.

Do you have a plunker that recreates the problem?

[PaulL1]

OK, #3138 is the same issue and has a plunker, the difference there is that there's a pinned column (selection row header). Is that also the case for you?

[neiz]

This seems to replicate my issue in that the column definitions are loaded later, after grid render (except I have no pinned columns) - I'm currently using unstable as well. Going to experiment with firing notifyDataChange for columns today and see if that forces a height recalc. I'll also give your workaround in #3138 a try as my use case does not need to toggle (dummy columns).

[marinovicir]

You should check - ui-grid-selection - the header size is calculated based on the first column. The selection button is the first one, as it does not have a filter it has a maximum size that does not include the filters, thus the explicitHeaderCanvasHeight is calculated to be the "Selection " header height.

For example I needed filters and they did not show in page(rc20).
I ended up with
.ui-grid-selection-row-header-buttons {
height : someDimension;
}
so I can force the display of filters.

All the planker examples work if you remove selection. Without dummy columns.

It does not matter if you fire notify or not, renderCanvas or not. it will just take the selection header height. At least on (rc20)

[c0bra]

@marinovicir (and @neiz) This was fixed with 7c5cdca. Can you try it with unstable?

[neiz]

@c0bra Just tried with unstable and seeing the same issue in my case. Here is a plunk of it working with the columnDefs loaded up front: http://plnkr.co/edit/DHTkZEKbzxXM2kBdoOuU?p=preview (this scenario has always worked for me)

However, when loaded after-the-fact, the height calculations are not reapplied:
http://plnkr.co/edit/AUlihNqWgvwU1WZ89nWG?p=preview

Using dummy columns to set the height (as opposed to just an []) is working for me though.

[marinovicir]

@c0bra Still not working even worse, the height is now 10px? http://plnkr.co/edit/w6ineaMrE0V9QC23tBga?p=preview

I don't think it's easy to fix this. Any way it is done, as long as the Selection Row Header is rendered first it will take that height. Because it will force the rest of the controls to render in that height.

I saw that the header is placed in a "left" render container, maybe those can be pushed to render after the body container. (at least the headers) I assume there is a right one also. It makes sense to render the body first as that is where the "action" will take place.

I did not look too much in the code, If I come up with a solution I will let you know.

Actually on second thought, why not just gridUtils.fakeElement for the header items. After that a max would work.

[c0bra]

Yes, sorry. This is my fault. The fix has not been pushed through the build process yet due to tests failing in IE. I am working on it.

[c0bra]

@marinovicir Can you check your plunker again? I had to include angular-touch for some reason but that could either be my setup or a separate issue.

[marinovicir]

@c0bra Hi, It works with a dummy column, don't know if it should work without it. What I did notice
Does not work

$scope.gridOptions = {
    enableFiltering: false,
    onRegisterApi : function(api){
      $scope.gridApi = api;
    }
  };

Does not work

  $scope.gridOptions = {
    enableFiltering: false,
    onRegisterApi : function(api){
      $scope.gridApi = api;
    },
    columnDefs : []
  };

Works

  $scope.gridOptions = {
    enableFiltering: false,
    onRegisterApi : function(api){
      $scope.gridApi = api;
    },
    columnDefs : [{name:'dummy'}]
  };

Works

  $scope.gridOptions = {
    enableFiltering: false,
    onRegisterApi : function(api){
      $scope.gridApi = api;
    },
    columnDefs : [{}]
  };

Notice in the 4th version all I did was declare an empty column. An it makes sense, ui-grid will take care of the defaults for that object. So maybe adding a 'default' empty column when using the selection functionality is a good workaround.

PS
Plunker

Maybe I am doing something wrong..

[marinovicir]

@c0bra
I think I found an easy fix for this, I noticed that the Selection header was present even if there were no custom columns, and it does not make sense to have only that header if I don't have any other columns, so in buildColumns I added a check for options.columnDefs, if there are columns from the user, add the extra headers, if not, don't display them.

  Grid.prototype.buildColumns = function buildColumns(opts) {
......

    // add headers only if we have columns added by user
    if (self.options.columnDefs.length > 0){
        //add row header columns to the grid columns array _after_ columns without columnDefs have been removed
        self.rowHeaderColumns.forEach(function (rowHeaderColumn) {
          self.columns.unshift(rowHeaderColumn);
        });
    }

PS. I always forget Plunker, it has a version with the fix. (Latest unstable, taken today)

[neiz]

I can confirm that using the fix proposed by @marinovicir remedies my situation as well.

[PaulL1]

I'm not convinced this is a great long-term solution, I'd see it as a workaround. Our header calculations need to be able to be called and correctly calculate header height irrespective of starting condition. What this is doing is manipulating the starting condition to be one that works, but presumably we'll still have other cases where it doesn't work and the header height will still need to be fixed.

[marinovicir]

I agree, It is not a long term solution. It just hides a problem in the calculations.

[marinovicir]

PS. Can`t edit in mobile. I still think the grid should not add headers if none are provided by the user. If the user does not know how the data will look. Why should the grid do anything.