Feature - Empty Base Layer #5687

louwie17 · 2016-09-21T12:17:19Z

This pull request adds a new feature 'emptyBaseLayer', that provides a background for the ui-grid, of continues rows, incase the height of the ui-grid is greater then the number of rows displayed.
This simulates a similar scenario as Excel, where the rows continue forever. The only exception is that the rows can't be edited, and just act as a background.
The rows/columns work with column resizing, and pinned rows.

A new tutorial page has been added. This is what it looks like in the tutorial:

mportuga

Please see my comments in the code review. Also, it would be helpful if you squashed your commits into one and followed our practices for commit comments so that your changes can show up in the change log. Personally I suggest using commitzen.
https://github.com/angular-ui/ui-grid/blob/master/CONTRIBUTING.md#-git-commit-guidelines

mportuga · 2016-09-21T13:48:15Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+         *  @description Enable empty base layer, which shows empty rows as background on the entire grid
+         *  <br/>Defaults to true, if the directive is used.
+         */
+        grid.options.enableEmptyGridBaseLayer = grid.options.enableEmptyGridBaseLayer !== false;


Won't this just overwrite the line above? (line 35 to be exact)

No, the two lines are different, I wanted to give the option of passing in true/false into the uiGridEmptyBaseLayer directive, like this: ui-grid-empty-base-layer="true/false". I wanted that to trump the grid.options value. That's why I check that before hand.
Unless you have a different suggestion?

I see. I can't think of another suggestion right now, but perhaps one of the other devs will.

mportuga · 2016-09-21T13:49:37Z

misc/tutorial/218_empty_grid_base_layer.ngdoc

+
+      $scope.randomSize = function () {
+        var newHeight = Math.floor(Math.random() * (300 - 100 + 1) + 300);
+        var newWidth = Math.floor(Math.random() * (600 - 200 + 1) + 200);


no need to use var here, you can use a comma to separate this line and the one above.

mportuga · 2016-09-21T13:50:52Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+         *  @name ui.grid.emptyBaseLayer.api:GridOptions
+         *
+         *  @description GridOptions for emptyBaseLayer feature, these are available to be
+         *  set using the ui-grid {@link ui.grid.class:GridOptions gridOptions}


Should this comment block be put above the grid.baseLayer object declaration?

mportuga · 2016-09-21T13:51:58Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+
+      setNumberOfEmptyRows: function(viewportHeight, grid) {
+        var rowHeight = grid.options.rowHeight;
+        var rows = Math.ceil(viewportHeight / rowHeight);


no need to use var here, you can use a comma to separate this line and the one above.

Will make that change!

mportuga · 2016-09-21T13:52:45Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+              }
+
+              var renderBodyContainer = uiGridCtrl.grid.renderContainers.body;
+              var prevGridHeight = renderBodyContainer.getViewportHeight();


no need to use var here, you can use a comma to separate this line and the one above.

mportuga · 2016-09-21T13:57:28Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+            var emptyBaseLayerContainer = $templateCache.get('ui-grid/emptyBaseLayerContainer');
+            $elm.prepend(emptyBaseLayerContainer);
+            return {
+              pre: function ($scope, $elm, $attrs, controllers) {


angular.noop might make more sense here since this function doesn't do anything.

mportuga · 2016-09-21T13:57:34Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+            return {
+              pre: function ($scope, $elm, $attrs, controllers) {
+              },
+              post: function ($scope, $elm, $attrs, controllers) {


angular.noop might make more sense here since this function doesn't do anything.

I did this as it seemed to be the standard for the other features, mostly looked at https://github.com/angular-ui/ui-grid/blob/master/src/features/expandable/js/expandable.js#L531

Makes sense. I guess this should stay this way then.

mportuga · 2016-09-21T13:59:16Z

src/features/empty-base-layer/less/emptyBaseLayer.less

+
+.ui-grid-viewport .ui-grid-empty-base-layer-container {
+  position: absolute;
+  height: 100% !important;


Do you need this important here? That could cause a lot for problems to users of ui-grid that are overwriting the default styles. Can this be done some other way?

The reason for using is because the same div inherits from 'ui-grid-canvas' as well, which gets a dynamic height/width, but I always wanted this div to take the full height of the viewport. So I should be able to find a different way.

That would be great, thanks!

mportuga · 2016-09-21T14:01:14Z

src/features/empty-base-layer/test/emptyBaseLayer.spec.js

+
+    var $compile = _$compile_;
+    scope = $rootScope;
+    timeout = $timeout;


Is this timeout variable really needed? It does not get used anywhere. Even $timeout itself is only used inside this beforeEach function.

mportuga · 2016-09-21T14:02:04Z

src/features/empty-base-layer/test/emptyBaseLayer.spec.js

+    $timeout(function () {
+      $compile(element)(scope);
+    });
+    $timeout.flush();


Why are you wrapping the compilation in a timeout? You could just as easily add a scope.$apply() function call after compiling the element instead of adding a timeout.

Probably the only reason for doing this was going of these tests: https://github.com/angular-ui/ui-grid/blob/master/src/features/expandable/test/expandable.spec.js#L29

I will try to fix those later in that case. But in the meantime there is no reason that new code should follow the same bad practice.

mportuga · 2016-09-21T14:33:51Z

src/features/empty-base-layer/js/emptyBaseLayer.js

+          emptyRows: []
+        };
+
+        grid.options.enableEmptyGridBaseLayer = baseLayerAttr !== "false";


undefined and null are both different than "false" couldn't that cause a problem? Is there a way for you to ensure that baseLayerAttr is a boolean when it comes in instead of a string?

background Made a new feature called 'emptyBaseLayer'. This can be added to the ui-grid by using ui-grid-empty-base-layer. This creates a fake background for the ui-grid of continues rows, the rows are not editable or clickable, and just act as a background. There also is a new tutorial page '218 Empty Base Layer'

louwie17 · 2016-09-22T12:12:33Z

@mportuga I updated your suggested changes, see what you think? Also I squashed all the commits, hopefully that looks a little better. It was my first time doing that actually, so I had some fun screwing up the rebasing here and there :)

mportuga reviewed Sep 21, 2016

View reviewed changes

mportuga added the waiting-response label Sep 21, 2016

mportuga reviewed Sep 21, 2016

View reviewed changes

louwie17 force-pushed the master branch from f84d541 to 61401cc Compare September 22, 2016 11:25

Lourens Schep added 2 commits September 22, 2016 09:06

refactor(emptyBaseLayer): remove unecessary new line

87618ac

louwie17 force-pushed the master branch from eb666be to 87618ac Compare September 22, 2016 12:07

mportuga added Looks Good To Me and removed waiting-response labels Sep 22, 2016

mportuga approved these changes Sep 22, 2016

View reviewed changes

dlgski added breaking and removed breaking labels Oct 19, 2016

dlgski merged commit 9397359 into angular-ui:master Oct 19, 2016

Feature - Empty Base Layer #5687

Feature - Empty Base Layer #5687

Uh oh!

Conversation

louwie17 commented Sep 21, 2016

Uh oh!

mportuga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louwie17 commented Sep 22, 2016

Uh oh!

Uh oh!