esvit / ng-table

Simple table with sorting and filtering on AngularJS
http://esvit.github.io/ng-table
BSD 3-Clause "New" or "Revised" License
2.77k stars 851 forks source link

can you please rename ngTableParams to NgTableParams #447

Closed sammiwei closed 9 years ago

sammiwei commented 9 years ago

It is a constructor, so upper case it would make most sense! It is breaking our lint. Thanks! Awesome work btw!

sctskw commented 9 years ago

you can mitigate this using jshint directives

/jshint newcap:false /

but, I agree. A nuisance

cpaof commented 9 years ago

You can also 'fix' this by using something like:

angular.module('myApp').factory('NgTableParams', ['ngTableParams', function(ngTableParams) {
  return ngTableParams;
}]);

...and then inject NgTableParams in your controller and using it as shown below...

angular.module('myApp').controller('myController', ['$scope', 'NgTableParams', function($scope, NgTableParams) {
  $scope.tableParams = new NgTableParams({
    // ...
  });
}]);

This could be integrated into the main source as well (with NgTableParams being the default and ngTableParams the legacy fallback). I'll create a pull request for it.

SoundLogic commented 9 years ago

should this be considered closed?

cpaof commented 9 years ago

Question is if this should not be fixed generally as suggested in my pull request 6a3ce60daa167f326e4328d069b41bba8c80cbe2

SoundLogic commented 9 years ago

ah yes didn't notice the commit was to a separate branch - just trying to familiarize myself with this project - seems pretty awesome, but not in 1.0 so I wanted to review the open issues

Nate-Wilkins commented 9 years ago

@SoundLogic Unfortonetly there are other conventions that we have to consider here. Some projects like to have all their services, factories, providers (injectables) lowerCamelCase which is what ngTableParams adheres too.

If it's really a problem I'd use @cpaof's solution or even

angular.run([
    'ngTableParams',

    function (ngTableParams) {
        var NgTableParams = ngTableParams;
    }
]);
SoundLogic commented 9 years ago

This is not an issue for me... I'm planning to use ng-table in a project so I've been going through the bugs to see if there's anything specific to my use cases. I was simply trying to be helpful and flag this as an item that had been resolved, but have since realized the code was committed to cpaof/ng-table and not esvit/ng-table :stuck_out_tongue_closed_eyes:

Nate-Wilkins commented 9 years ago

Ahhh I see @sammiwei or @esvit could you close this issue?