-
Notifications
You must be signed in to change notification settings - Fork 307
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
SB-868 - Announcement attachment up loader refactoring with injectable service/factory #100
SB-868 - Announcement attachment up loader refactoring with injectable service/factory #100
Conversation
nilesh-more
commented
Nov 21, 2017
•
edited
Loading
edited
- Announcement attachment up loader refactoring with injectable service/factory
- style doc added
- gulp test - done
…jectable service/factory
…3' into refactoring
closing PR for cleaning repo |
angular.module('playerApp').controller('createAnnouncementCtrl', ['$rootScope', '$scope', '$timeout', 'config', 'toasterService', 'announcementService', '$filter', 'uuid4', | ||
function ($rootScope, $scope, $timeout, config, toasterService, announcementService, $filter, uuid4) { | ||
angular.module('playerApp').controller('createAnnouncementCtrl', ['$rootScope', '$scope', '$timeout', 'config', 'toasterService', 'announcementService', '$filter', 'uuid4', 'fileUpload', | ||
function ($rootScope, $scope, $timeout, config, toasterService, announcementService, $filter, uuid4, fileUpload) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$services should be placed before others, please move $filter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kochhar resolved
All '$' item should be injected first. cc: @harishgilimi
* @desc - remove selected recipients | ||
* @memberOf Controllers.createAnnouncementCtrl | ||
* @param {object} [item] [current selected item] | ||
*/ | ||
createAnn.removeRicipients = function (item) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp: ricipients > recipients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kochhar resolved
* @param {Object} option - Option object to invoke controller callback function | ||
* @returns {Callback} Trigger callback function | ||
*/ | ||
createFineUploadInstance: function (options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
having the uploader type name in the method does not prevent strong coupling. having the options sent from the controller doesn't prevent abstraction leak.
the factory method should be called when the app is initialised. app-level config options should be passed in and an instance created. this instance is available as a partially configured factory. this factory can then be called with controller specific options to get the uploader instance.
@@ -94,14 +94,14 @@ describe('Controller: createAnnouncementCtrl', function () { | |||
}) | |||
|
|||
it('convert file size into KB / MB', function (done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing expectations here...
spyOn(createAnn, 'convertFileSize').and.callThrough() | ||
createAnn.convertFileSize(0) | ||
spyOn(createAnn, 'getReadableFileSize').and.callThrough() | ||
createAnn.getReadableFileSize(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and here.
…ease-1.3' into refactoring
…ease-1.3' into refactoring
Issue-#SB-25922:Fix-Matrix Instance color on completion
UPHRH ISSUE 4899 While editing the batch for PIAA assessment, Udate api is throwing 404 error issue fixed