Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

feat(articles): Modify articles module to implement style guidelines. #1126

Merged
merged 1 commit into from
Jan 15, 2016
Merged

feat(articles): Modify articles module to implement style guidelines. #1126

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
9 changes: 6 additions & 3 deletions modules/articles/client/articles.client.module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
'use strict';
(function (app) {
'use strict';

// Use Applicaion configuration module to register a new module
ApplicationConfiguration.registerModule('articles');
app.registerModule('articles');
app.registerModule('articles.services');
app.registerModule('articles.routes', ['ui.router', 'articles.services']);
})(ApplicationConfiguration);
18 changes: 11 additions & 7 deletions modules/articles/client/config/articles.client.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
'use strict';
(function () {
'use strict';

// Configuring the Articles module
angular.module('articles').run(['Menus',
function (Menus) {
// Add the articles dropdown item
angular
.module('articles')
.run(menuConfig);

menuConfig.$inject = ['Menus'];

function menuConfig(Menus) {
Menus.addMenuItem('topbar', {
title: 'Articles',
state: 'articles',
Expand All @@ -19,9 +23,9 @@ angular.module('articles').run(['Menus',

// Add the dropdown create item
Menus.addSubMenuItem('topbar', 'articles', {
title: 'Create Articles',
title: 'Create Article',
state: 'articles.create',
roles: ['user']
});
}
]);
})();
61 changes: 48 additions & 13 deletions modules/articles/client/config/articles.client.routes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
'use strict';
(function () {
'use strict';

// Setting up route
angular.module('articles').config(['$stateProvider',
function ($stateProvider) {
// Articles state routing
angular
.module('articles.routes')
.config(routeConfig);

routeConfig.$inject = ['$stateProvider'];

function routeConfig($stateProvider) {
$stateProvider
.state('articles', {
abstract: true,
Expand All @@ -12,25 +16,56 @@ angular.module('articles').config(['$stateProvider',
})
.state('articles.list', {
url: '',
templateUrl: 'modules/articles/client/views/list-articles.client.view.html'
templateUrl: 'modules/articles/client/views/list-articles.client.view.html',
controller: 'ArticlesListController',
controllerAs: 'vm'
})
.state('articles.create', {
url: '/create',
templateUrl: 'modules/articles/client/views/create-article.client.view.html',
templateUrl: 'modules/articles/client/views/form-article.client.view.html',
controller: 'ArticlesController',
controllerAs: 'vm',
resolve: {
articleResolve: newArticle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first glance I thought these all resolved to the same thing. @trainerbill says it saves a little code, but it isn't clear and it makes more sense to just call this the 1-2x necessary in the controller. @mleanos @rhutchison @lirantal @codydaig any other suggestions? This saves a couple lines but it is easy mistaken for the same injection and it just moves the code if it is only used one or two times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mind how it is, now that I understand it. When I was looking at this a while back, it was really confusing to me. I don't think it's necessary, but it's probably ok. However, I know others will be confused as well; especially when just looking at the controller code.

It does feel a little weird to have logic, that seemingly belongs in the controller, in the route config when it's not really resolving any data; but just an empty new instance of an Article.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand the concern? We are resolving a new instance of an article which is being injected to the controller. Unless you want to have a separate controller for create vs edit/view, you need to resolve something. One can argue that you can resolve null and then check for null in the controller and instantiate there, but what is the point when you can avoid it? Adding another controller will just duplicate code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhutchison in one place, you resolve newArticle -> articleResolve, and in another getArticle -> articleResolve, and they are both referenced as article in the controller...that's confusing because you have to know the state as well when you are writing code. I'd rather not do that.

For reference L29 and L41 in this file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They both return a model of the same type - one is instantiated, the other is retrieved from the server.

articleResolve is Article in both cases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here so people diving into the code knows what is going on?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think it makes sense just to do this in the controller. It's less confusing and self-documenting that way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One additional benefit is to share the resolved value among nested views. I use nested views all the time, so if I pass the Article in the resolve then populate it in one controller that value gets shared among my other views and controllers. If you instantiate the model in the controller the nested views would not get that data. I think we should start looking at nested views for much of the project and this is a good start.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trainerbill I think I like what you're saying. However, I have a couple questions..

  1. Are you sharing the resolved data among multiple child states?
  2. Do your nested views have their own states, and/or controllers?

Can you elaborate on the use case of your nested views, or give an example (or gist) of what this implementation looks like?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yep
  2. Sometimes. You can define a new controller otherwise it inherits the parents. Also inherits all the resolves.

This is just off the top of my head:

https://gist.github.com/trainerbill/f51897d1a1e9ab5eaf26

So in my layout.html default server file i have

and ui-view="main"

I target main:

https://gist.github.com/trainerbill/f51897d1a1e9ab5eaf26#file-gistfile1-txt-L12

And put a layout file in it. This is a 2-4 column grid with 4 rows all with new ui-views like fluid-col-1-row-1 that I can now target in my nested states. Its tough to explain because articles is basic, but if we also wanted to save references with the article then we can have separate views that show the reference information and since it is using articleResolve it is shared so when the main article is saved so are the references.

Here is an abstract route that just resolves the article:
https://gist.github.com/trainerbill/f51897d1a1e9ab5eaf26#file-gistfile1-txt-L48

Now on the edit route my one view has the SaveArticleController which has the saving logic
https://gist.github.com/trainerbill/f51897d1a1e9ab5eaf26#file-gistfile1-txt-L84

and the references has no controller because it is using the ResolveArticleController, which just sets vm.article = articleResolve and so now both views have article resolve. If I update anything in one the other gets the update.

I think we need to move to nested views across the whole project and need to bring it up for discussion. It allows you to reuse views all over the place. For instance if I have a author module then I can use the view article easily, by resolving the article in the route and copy and pasting the view. done.

},
data: {
roles: ['user', 'admin']
}
})
.state('articles.view', {
url: '/:articleId',
templateUrl: 'modules/articles/client/views/view-article.client.view.html'
})
.state('articles.edit', {
url: '/:articleId/edit',
templateUrl: 'modules/articles/client/views/edit-article.client.view.html',
templateUrl: 'modules/articles/client/views/form-article.client.view.html',
controller: 'ArticlesController',
controllerAs: 'vm',
resolve: {
articleResolve: getArticle
},
data: {
roles: ['user', 'admin']
}
})
.state('articles.view', {
url: '/:articleId',
templateUrl: 'modules/articles/client/views/view-article.client.view.html',
controller: 'ArticlesController',
controllerAs: 'vm',
resolve: {
articleResolve: getArticle
}
});
}
]);

getArticle.$inject = ['$stateParams', 'ArticlesService'];

function getArticle($stateParams, ArticlesService) {
return ArticlesService.get({
articleId: $stateParams.articleId
}).$promise;
}

newArticle.$inject = ['ArticlesService'];

function newArticle(ArticlesService) {
return new ArticlesService();
}
})();
106 changes: 37 additions & 69 deletions modules/articles/client/controllers/articles.client.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,84 +1,52 @@
'use strict';
(function () {
'use strict';

// Articles controller
angular.module('articles').controller('ArticlesController', ['$scope', '$stateParams', '$location', 'Authentication', 'Articles',
function ($scope, $stateParams, $location, Authentication, Articles) {
$scope.authentication = Authentication;
angular
.module('articles')
.controller('ArticlesController', ArticlesController);

// Create new Article
$scope.create = function (isValid) {
$scope.error = null;
ArticlesController.$inject = ['$scope', '$state', 'articleResolve', 'Authentication'];

if (!isValid) {
$scope.$broadcast('show-errors-check-validity', 'articleForm');

return false;
}

// Create new Article object
var article = new Articles({
title: this.title,
content: this.content
});
function ArticlesController($scope, $state, article, Authentication) {
var vm = this;

// Redirect after save
article.$save(function (response) {
$location.path('articles/' + response._id);

// Clear form fields
$scope.title = '';
$scope.content = '';
}, function (errorResponse) {
$scope.error = errorResponse.data.message;
});
};
vm.article = article;
vm.authentication = Authentication;
vm.error = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't it initialize error and form by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but you would only know that by reading through all of the views. Listing it in the controller makes it easy to read and helps you instantly identify which members of the controller can be bound and used in the View.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on what Ryan is saying.

vm.form = {};
vm.remove = remove;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just set the property = to the function definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref: Bindable Members Up Top

Setting anonymous functions in-line can be easy, but when those functions are more than 1 line of code they can reduce the readability. Defining the functions below the bindable members (the functions will be hoisted) moves the implementation details down, keeps the bindable members up top, and makes it easier to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I a huge fan of this way of defining memebers. I use this in a couple of my projects. The code is so much easier to read, and manage.

vm.save = save;

// Remove existing Article
$scope.remove = function (article) {
if (article) {
article.$remove();

for (var i in $scope.articles) {
if ($scope.articles[i] === article) {
$scope.articles.splice(i, 1);
}
}
} else {
$scope.article.$remove(function () {
$location.path('articles');
});
function remove() {
if (confirm('Are you sure you want to delete?')) {
vm.article.$remove($state.go('articles.list'));
}
};

// Update existing Article
$scope.update = function (isValid) {
$scope.error = null;
}

// Save Article
function save(isValid) {
if (!isValid) {
$scope.$broadcast('show-errors-check-validity', 'articleForm');

$scope.$broadcast('show-errors-check-validity', 'vm.form.articleForm');
return false;
}

var article = $scope.article;

article.$update(function () {
$location.path('articles/' + article._id);
}, function (errorResponse) {
$scope.error = errorResponse.data.message;
});
};
// TODO: move create/update logic to service
if (vm.article._id) {
vm.article.$update(successCallback, errorCallback);
} else {
vm.article.$save(successCallback, errorCallback);
}

// Find a list of Articles
$scope.find = function () {
$scope.articles = Articles.query();
};
function successCallback(res) {
$state.go('articles.view', {
articleId: res._id
});
}

// Find existing Article
$scope.findOne = function () {
$scope.article = Articles.get({
articleId: $stateParams.articleId
});
};
function errorCallback(res) {
vm.error = res.data.message;
}
}
}
]);
})();
15 changes: 15 additions & 0 deletions modules/articles/client/controllers/list.articles.client.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
(function () {
'use strict';

angular
.module('articles')
.controller('ArticlesListController', ArticlesListController);

ArticlesListController.$inject = ['ArticlesService'];

function ArticlesListController(ArticlesService) {
var vm = this;

vm.articles = ArticlesService.query();
}
})();
15 changes: 10 additions & 5 deletions modules/articles/client/services/articles.client.service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
'use strict';
(function () {
'use strict';

//Articles service used for communicating with the articles REST endpoints
angular.module('articles').factory('Articles', ['$resource',
function ($resource) {
angular
.module('articles.services')
.factory('ArticlesService', ArticlesService);

ArticlesService.$inject = ['$resource'];

function ArticlesService($resource) {
return $resource('api/articles/:articleId', {
articleId: '@_id'
}, {
Expand All @@ -11,4 +16,4 @@ angular.module('articles').factory('Articles', ['$resource',
}
});
}
]);
})();
28 changes: 0 additions & 28 deletions modules/articles/client/views/create-article.client.view.html
View file
Open in desktop

This file was deleted.

28 changes: 0 additions & 28 deletions modules/articles/client/views/edit-article.client.view.html
View file
Open in desktop

This file was deleted.

28 changes: 28 additions & 0 deletions modules/articles/client/views/form-article.client.view.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<section>
<div class="page-header">
<h1>{{vm.article._id ? 'Edit Article' : 'New Article'}}</h1>
</div>
<div class="col-md-12">
<form name="vm.form.articleForm" class="form-horizontal" ng-submit="vm.save(vm.form.articleForm.$valid)" novalidate>
<fieldset>
<div class="form-group" show-errors>
<label class="control-label" for="title">Title</label>
<input name="title" type="text" ng-model="vm.article.title" id="title" class="form-control" placeholder="Title" required>
<div ng-messages="vm.form.articleForm.title.$error" role="alert">
<p class="help-block error-text" ng-message="required">Article title is required.</p>
</div>
</div>
<div class="form-group">
<label class="control-label" for="content">Content</label>
<textarea name="content" data-ng-model="vm.article.content" id="content" class="form-control" cols="30" rows="10" placeholder="Content"></textarea>
</div>
<div class="form-group">
<button type="submit" class="btn btn-default">{{vm.article._id ? 'Update' : 'Create'}}</button>
</div>
<div ng-show="vm.error" class="text-danger">
<strong ng-bind="vm.error"></strong>
</div>
</fieldset>
</form>
</div>
</section>
6 changes: 3 additions & 3 deletions modules/articles/client/views/list-articles.client.view.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
<section ng-controller="ArticlesController" ng-init="find()">
<section>
<div class="page-header">
<h1>Articles</h1>
</div>
<div class="list-group">
<a ng-repeat="article in articles" ui-sref="articles.view({articleId: article._id})" class="list-group-item">
<a ng-repeat="article in vm.articles" ui-sref="articles.view({ articleId: article._id })" class="list-group-item">
<small class="list-group-item-text">
Posted on
<span ng-bind="article.created | date:'mediumDate'"></span>
Expand All @@ -15,7 +15,7 @@ <h4 class="list-group-item-heading" ng-bind="article.title"></h4>
<p class="list-group-item-text" ng-bind="article.content"></p>
</a>
</div>
<div class="alert alert-warning text-center" ng-if="articles.$resolved && !articles.length">
<div class="alert alert-warning text-center" ng-if="vm.articles.$resolved && !vm.articles.length">
No articles yet, why don't you <a ui-sref="articles.create">create one</a>?
</div>
</section>
Loading