From 1668a78bc4baed07ef064b62d31861075813e55b Mon Sep 17 00:00:00 2001
From: Aaron Krause <aaronjkrause@gmail.com>
Date: Fri, 29 Jul 2016 10:25:20 -0700
Subject: [PATCH] project comments

---
 app/index.html                     |  8 ++++++++
 app/js/directives/index.js         | 10 ++++++++++
 app/js/services/profile-service.js |  4 ++++
 3 files changed, 22 insertions(+)
diff --git a/app/index.html b/app/index.html
index 41aaf9d..40ec267 100644
--- a/app/index.html
+++ b/app/index.html
@@ -10,7 +10,15 @@
   </head>
   <body ng-app="BucketListApp">
     <div class="content">
+    <!-- If you're using the AuthController in your nav logic and that's all encapsulated in
+      this header I would start by putting the whole header into a directive and then handling
+      Auth based stuff from the controller in the directive itself. When you set a controller
+      out here in your main view like this and then refer to it by a name you set here in your
+      directive template it breaks encapsulation and could be more confusing in the long run. -->
       <header ng-controller="AuthController as ac">
+        <!-- You can also set classes based on an evaluated expression (see: ng-class) I realize
+        you're doing this with media queries so that may be a little complicated but if it gives
+        you the chance to better simplify and encapsulate your logic it may be worth figuring out. -->
         <nav class="full-nav">
           <ul>
             <li><a href="#/"><button type="submit">Home</button></a></li>
diff --git a/app/js/directives/index.js b/app/js/directives/index.js
index db46f05..2103842 100644
--- a/app/js/directives/index.js
+++ b/app/js/directives/index.js
@@ -1,5 +1,15 @@
 'use strict';
 
+//Overall it seems like your use of directives boils down to them being HTML partials rather
+//than encapsulated components. In some ways this is even a little closer to the way that
+//directives may have been done once but when it comes down to it it makes the directives
+//themselves a little unnecessary. I would consider going one way or the other. Breaking some
+//of the html out into views and having routing handle them or taking some of the controllers
+//that you have them interacting with and making them properties of the directives. You should
+//generally count on directives having an isolate scope, because conceptually that means that
+//you only have the functionality that you've added to them and you only have attributes that
+//you've passed in which makes them encapsulated pieces of your view and it's associated logic.
+
 module.exports = function(app) {
   require('./blog-full-view-directive')(app);
   require('./blog-list-directive')(app);
diff --git a/app/js/services/profile-service.js b/app/js/services/profile-service.js
index 9b571ef..a7da200 100644
--- a/app/js/services/profile-service.js
+++ b/app/js/services/profile-service.js
@@ -16,6 +16,10 @@ module.exports = function(app) {
       })
       .then((res) => {
         service.profile = res.data;
+        //this could potentially crash your stuff if there's no function passed into
+        //getProfile. Even if you know you're the one using it it's a good idea to check
+        //for that. You could use an if() or just cb && cb(). You could also catch the error
+        //with a catch chained onto this then but it's probably better just to check sooner.
         cb();
       }, ErrorService.logError('Error on profile'));
     };
