Skip to content

Commit

Permalink
Fixes #399, making cell editing work again
Browse files Browse the repository at this point in the history
This is an issue with how src/ng-input.js is handling the value of the ng-input directive. The current default template contains an $eval, which means that when ng-input.js runs a $parse on it, it gets the evaluated value of the cell itself. For a string, (which is an object) this ends up not actually modifying the cell contents but at least it doesn't fire an exception. For an integer primitive you would end up with an undefined setter and a noop getter, which would throw exceptions when you try to use undefined as a function to set the cell value.

The fix, as I see it, is to use a regex to strip out the $eval statement from the ng-input in the editable cell template.

I've also cleaned up ng-input.js, added some comments, and added handling
for when the enter key is used to modify a cell's contents
  • Loading branch information
c0bra committed May 8, 2013
1 parent afa1cc9 commit 9f78401
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 23 deletions.
50 changes: 34 additions & 16 deletions src/directives/ng-input.js
Original file line number Diff line number Diff line change
@@ -1,35 +1,53 @@
ngGridDirectives.directive('ngInput',['$parse', function($parse) {
return function ($scope, elm, attrs) {
var getter = $parse($scope.$eval(attrs.ngInput));
return function ($scope, elm, attrs) {

// Strip the $eval off of the ng-input tag, we want WHERE to put the edited values, not the actual edited/displayed value itself
var inputter = attrs.ngInput.replace(/.+?\((.+?)\)/, '$1');

var evaled = $scope.$eval(inputter);
var getter = $parse(evaled);
var setter = getter.assign;
var oldCellValue = getter($scope.row.entity);
var oldCellValue = getter($scope);

elm.val(oldCellValue);
elm.bind('keyup', function() {
var newVal = elm.val();
if (!$scope.$root.$$phase) {
$scope.$apply(function(){setter($scope.row.entity,newVal); });
}
});

elm.bind('keyup', function() {
var newVal = elm.val();
// dump(newVal);
if (!$scope.$root.$$phase) {
$scope.$apply(function(){
setter($scope, newVal);
});
}
});

elm.bind('keydown', function(evt){
switch(evt.keyCode){
case 37:
case 38:
case 39:
case 40:
case 37: // Left arrow
case 38: // Up arrow
case 39: // Right arrow
case 40: // Down arrow
evt.stopPropagation();
break;
case 27:
case 27: // Esc (reset to old value)
if (!$scope.$root.$$phase) {
$scope.$apply(function(){
setter($scope.row.entity,oldCellValue);
setter($scope, oldCellValue);
elm.val(oldCellValue);
elm.blur();
});
}
case 13: // Enter (apply new value)
if (!$scope.$root.$$phase) {
$scope.$apply(function(){
setter($scope, elm.val());
elm.blur();
});
}
default:
break;
}
return true;
});
};
};
}]);
37 changes: 30 additions & 7 deletions test/unit/directivesSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,19 +126,22 @@ describe('directives', function () {
});

describe('column', function () {
describe('enableCellEdit option', function () {
it('should allow colDef.enableCellEdit to override config.enableCellEdit', inject(function ($rootScope, $compile) {
describe('cell editing', function () {
var elm, element, $scope;

beforeEach(inject(function ($rootScope, $compile) {
$scope = $rootScope;
elm = angular.element(
'<div ng-grid="gridOptions" style="width: 1000px; height: 1000px"></div>'
);

$rootScope.myData = [{name: "Moroni", age: 50},
$scope.myData = [{name: "Moroni", age: 'test' },
{name: "Tiancum", age: 43},
{name: "Jacob", age: 27},
{name: "Nephi", age: 29},
{name: "Enos", age: 34}];

$rootScope.gridOptions = {
$scope.gridOptions = {
data: 'myData',
enableCellEdit: true,
columnDefs: [
Expand All @@ -147,9 +150,11 @@ describe('directives', function () {
]
};

var element = $compile(elm)($rootScope);
$rootScope.$digest();
element = $compile(elm)($scope);
$scope.$digest();
}));

it('should allow colDef.enableCellEdit to override config.enableCellEdit', function() {
var cell = element.find('.ngRow:eq(0) .ngCell.col0 [ng-dblclick]');

// Have to use jasmine's async testing helpers to get around the setTimeout(..., 0) in ng-cell-has-focus.editCell()
Expand All @@ -160,7 +165,25 @@ describe('directives', function () {
runs(function() {
expect(cell.find('input').length).toEqual(0);
});
}));
});

it('should not throw an exception when the column contains a non-string', function() {
var cell = element.find('.ngRow:eq(0) .ngCell.col1 [ng-dblclick]');

runs(function() {
browserTrigger(cell, 'dblclick');
});
waits(200);
runs(function() {
expect(function(){
// Trigger the input handler
var input = cell.find('input');
input.triggerHandler('keyup');
}).not.toThrow();
});
});

// TODO: add a test for enter key modifying the inputted contents
});

describe('displayName option', function() {
Expand Down

3 comments on commit 9f78401

@swalters
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this fix work with complex properties, like person.name? I think the eval logic had stopped that from working.

@c0bra
Copy link
Contributor Author

@c0bra c0bra commented on 9f78401 May 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swalters good question; I will add a test for that.

@c0bra
Copy link
Contributor Author

@c0bra c0bra commented on 9f78401 May 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@swalters yep it works: 4a5c93d

Please sign in to comment.