Grzegorz Judas#9
Conversation
|
Do we really want to merge commit named "Grzegorz Judas"? |
kapke
left a comment
kapke
left a comment
There was a problem hiding this comment.
Branch called grzegorz-judas? Does that branch contain Grzegorz Judas?
Every sample has its own directory and that is very to see how specific code is working. So divide it into directories and provide example of using specific directive. Without example it doesn't make sense for anyone. As these examples are in AngularJS, you can rename directory angular-edrna into just angular.
| @@ -0,0 +1,174 @@ | |||
| 'use strict'; | |||
|
|
|||
| var refreshData; | |||
There was a problem hiding this comment.
Why global variable? It's antipattern and should not be published as sample of good code.
|
|
||
| var refreshData; | ||
|
|
||
| angular.module('edrnaApp').directive('reorderWrapper', ['$timeout', '$compile', '$document', |
| $scope.reorderComplete = typeof $scope.reorderComplete === 'function' ? $scope.reorderComplete : new Function(); | ||
|
|
||
| $scope.$on('draggable:start', function(scope, e) { | ||
| var _handler = angular.element(e.event.target); |
There was a problem hiding this comment.
Why some variables are prefixed with _ and some not?
| } | ||
| }); | ||
|
|
||
| $scope.$on('draggable:end', function(e, d) { |
There was a problem hiding this comment.
I know that e is an event. I have no idea what is d
| $scope.reorderComplete(angular.copy($scope.reorderData), originalElementPosition, overallDiff); | ||
| }); | ||
|
|
||
| var getElements = function(callback) { |
There was a problem hiding this comment.
Why not function getElements () {}? That's much better than var getElements = function () {}.
- Hoisting
- Easier to read due to different code highlight
- Easier to debug due to function name in stack traces
| var getElements = function(callback) { | ||
| if(typeof callback !== 'function') callback = new Function(); | ||
|
|
||
| $timeout(function() { |
There was a problem hiding this comment.
It's unclear why that timeout is used here
| } | ||
|
|
||
| function SubConverterCfg(SubConverterProvider) { | ||
| SubConverterProvider.addSrcConverter('application/x-subrip', function(data) { |
There was a problem hiding this comment.
I would add every converter in separate file. That would be much more readable.
| return result; | ||
| }); | ||
| } | ||
| })(); |
There was a problem hiding this comment.
I don't see any usage example. I think that would be great to see how people can use it.
| } | ||
| }); | ||
|
|
||
| lines = lines.filter(function(l) { return true; }); |
| var parsed = []; | ||
| var lines = data.split('</text>'); | ||
| var tagRegex = /^<text start="([0-9]+(.|)([0-9]+|))" dur="([0-9]+(.|)([0-9]+|))">/; | ||
| // lines = lines.filter(function(line) { return line.match(tagRegex) !== null; }); |
| return supportedDst.indexOf(mime) !== -1; | ||
| } | ||
|
|
||
| function converter() { |
There was a problem hiding this comment.
That function is a constructor function and it's name should start from upper cased C as for rest of constructors there
2 participants
No description provided.