17

Avoiding the fact this code appears to be off-topic (because you mention it's incomplete), I still think it would prompt an interesting review. I'm so pretty Let's first address some superficial concerns: You have an inconsistent use of semi-colons. Using semi-colons now is more or less a stylistic choice due to ASI, however if you're going to use semi-...


14

Well, there's lots of code needing review. I've done my best to split it into each language. I've avoided reviewing Angular, since it's a library I'm not comfortable with. HTML: You have tons and tons of ng-* properties. That is invalid HTML. You must use data-ng-*. According to the documentation, data-ng-* is valid and will be detected. Still using <...


14

Tables for layout This is a clear and obvious misuse of the table element. Tables are for representing tabular data and that's it. Would that same content make sense in a spreadsheet? If the answer is no (as it is in this case), then don't use a table. Use CSS for controlling your layout instead. Excessive IDs You don't have a single non-id selector ...


13

I like it, you could clean up the commented out code and possibly replace $window.googleMapsInitialized = function () { mapsDefer.resolve(); }; with $window.googleMapsInitialized = mapsDefer.resolve; Personally, I would re-arrange the code a tiny bit, keep the URL completely on top, keep asyncLoad and the call closer together. Keep ...


13

There's not much to cover, other than the things @IsmaelMiguel went over, nonetheless: The Mundane: This is less of a important thing, as a style thing, but: font-family: Georgia, Times, "Times New Roman", serif; font-size: 1.6em; text-align: center; background-color: #0033CC; color: #EEEEEE; border-top-color: #FFFFFF; vertical-align: middle; I find it ...


13

Background: I work as a web developer at a company. I'm the only Angular guy here, and we are using Angular in our greenfield CRM project, so my answer will mainly address Angular (as no one else has here). id attributes First thing is first - you're using lots of id attributes. Generally speaking, you only want to use id when it Is a unique 'object' in ...


11

A few notes: I am confused as to why you need AngularJS for this. I prefer to use the bitwise & with 255 instead of 0xFF in this case. Your function can't handle non-hex characters such as #. I would remove them before parsing. You can simplify your creation of the RGB string by using the join() method. Final function: function hexToRgb(hex) { ...


10

From a once over: It makes more sense from a UX experience to create the box on click then on mousedown There are a number of indenting inconsistencies, use TidyUp in jsFiddle to fix that Naming is good Flow of the code is good Commenting is good I don't like the constants 25 and 50 all over the place, name them and manage them You have a few place where ...


9

You should use strict mode in your JS scripts. It will allow you to avoid a lot of common mistakes. See the section "Changes in strict mode" in the MDN article I linked. Some of the advantages of using strict mode: Disallows global variables (when dev "forgets" to declare them); Disallows duplicate variables (such as declaring ENTER_KEY twice); Function ...


8

This is a funny question because One of the most readable Angular submissions on CR There is probably too much logic in the controller indeed Some observations: It seems the controller knows too much, it should not know that 5 is the maximum amount of items, or that 1 is the minimum amount of items to be kept It seems silly to pass the list to list every ...


8

$scope.user = gon.current_user What's gon? I assume it's your authentication/session object with all the things the app needs before running. Best if you put it inside a service or a value so that you can easily use dependency injection with it. That way, your controllers are uniform and only depend on stuff that comes from dependencies, not from ...


7

Generally speaking, you should avoid looking outside the current directive for anything that would affect your internal state (bar events). The preferred method of communication is scopes. Here is how I would recommend you do this. <div ng-controller="MyCtrl"> <div> <input ng-readonly="is_readonly" /> </div> <...


7

I think these are different layers of responsibility. The service.get('api.example.com/products'); is called in the controller to the corresponding view. What I suggest is the following: You have a common folder, where all your shared services and functions are placed. Inside this, you create following services: api.js // to handle all your CRUD calls ...


7

Here are a few general feedbacks: Have a coherent code style You're mixing multiple code styles sometimes you put spaces around operators ng-show='step == 0', sometimes you don't ng-show='step==2' sometimes you put braces on same line $scope.checkWords = function(){, sometimes you don't $scope.nextLetter = function() { Coherent code style improves ...


6

There is much wrong with your code.. You are iterating over every record to find a username, what happens if you have lots of users ? You are not verifying the password !? It seems like you are exposing the REST services with all the user id's and passwords !? It seems that if the user knows the /loggedin URL, then the user can skip logging in since you do ...


6

At first sight, your code is short and there is nothing wrong with it. At second sight, there are a few things to improve: Your semicolons are all over the place; you have both missing and pointless semicolons. There is no point in declaring id in all since you clearly will not use it ngPost is an old skool constructor, it's name really should start with ...


6

You're attempting to handle the data binding yourself by creating arrays to store objects based on the click event. Angular does the two way binding for you. Basically, I assign a new property named "included" by assigning it as the model to the input checkbox. Then I use that property on the filter of the fruit array to display only the one's whose ...


6

AngularJS encourages slim controllers. They'd rather you have fat directives and services than controllers. I think you are absolutely correct to have many directives. In my angular apps, I have some directives without link to make my templates clean and readable. You can compress trees of HTML into their high-level meaning. Plus, if things get more ...


6

Nice work! I presume the complicated way of getting the right element measurements is to cater for many browsers, won't comment on that. I would add some comments on the nature of the 'magic number' scope.triggerOffset = 150; or, better, encapsulate it away as .constant or offer as configurable option, to make your directive more re-usable. I find the use ...


6

Using an IIFE seems to be the tidiest way I've found to do it. This allows you to define your constant as a function (that is immediately invoked) that returns your constant object so you can use it as normal, i.e: LABELS.appName .constant('LABELS', (function () { var self = {}; self.appName = 'MyApp'; self.companyName = 'MyCompany'; self....


6

welcome to CodeReview :-) Firstly, I would mention that your $item and $model parameters should just be item and model. The dollar-sign prefix is conventionally reserved for official vendor libraries from Angular, and should not be used in your code to prevent confusion. I initially thought $model was a derivative of ngModel! Secondly, do you need ...


6

Aside from the issue of setIntervals resolution/accuracy there's a lot of room for improvement in your angular. angular.module('app', []); "app" isn't a good name for your module "fruitjs.timer" would be better as it is more descriptive and has a much lower chance of naming clashes. app.controller('main', function($scope, $interval) { Again "main" isn't ...


6

Very nice. I can't really see any major issues here, just some nits to pick. Note, though, that my knowledge of Angular is near-nil, so perhaps there's something there that someone else will be better equipped to comment on. Markup The markup is all-Bootstrap, so it's pretty by-the-book already. There's a lot of it though, making it a hard to skim. To get ...


6

First of all, you should be aware that "first name" and "last name" are very tricky concepts. An example in that linked Wikipedia article is José Luis Rodríguez Zapatero. José Luis is the first name, though it's in two words. Rodríguez Zapatero are both surnames (paternal and maternal). Or you have cultures where "name" means surname first and then given ...


5

This took a while to parse, it's probably me, I found few flaws with the code, except for a distinct lack of comments. I would have made url in preload an optional, 2nd parameter, I would probably want to call preload a few times with different URL's ( for 'data' ) I think using a switch for media and data is overkill, I would have done a simple if ...


5

Some remarks and refinements of previous answer but too long to put in the comments. Instead of <button ng-click="addToList()">Add to List</button> I would make it slightly more expressive and re-usable: <button ng-click="addToList(todo)">Add to List</button> That is also consistent with remove($index), which I would rename to ...


5

If I understand correctly, you can unnest your code, and catch any possible error at the end. That's the idea of using promises: websiteService.get('websites').then(function(data) { $scope.websites = data return websiteService.get('groups') }).then(function(data) { $scope.groups = data return websiteService.get('websites_groups') }).then(function(...


5

Angular is not exactly a MVC framework, it's a MVW - Model-View-Whatever. You can find an explanation here. The application has a good structure: Services to retrieve data from server Controller to access the services and provide the view model Html for presentation A good solution to work with Restful in angular is Restangular. I would recommend you ...


5

Anything that makes each instance of a directive different should be delegated somewhere higher up. You make a "wrapper" directive. Generic logic stays down the hierarchy, while specific logic wraps and provides for it. Assuming each of your directive has a hierarchy of something like: page -> your three directives -> ui-select I suggest you ...


5

I'd actually go a bit further than janos. He is perfectly right in that by repeatedly checking rejection.status, you're repeating yourself in code. Repeatedly checking the variable against multiple values makes this a perfect candidate for a switch statement: responseError: function(rejection) { if (rejection.status === 401) { switch (rejection....


Only top voted, non community-wiki answers of a minimum length are eligible