Skip to content

project comments #159

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.

Sign up for GitHub

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

Open
aaron-j-krause wants to merge 1 commit into
base: staging
Choose a base branch
from
Open

project comments #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions app/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
10 changes: 10 additions & 0 deletions app/js/directives/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
Expand Down
4 changes: 4 additions & 0 deletions app/js/services/profile-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
};
Expand Down