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

rhutchison · 2016-01-01T20:10:34Z

Update the articles module to implement the style guidelines.

Much of this work is from @trainerbill - I sent him a PR so he can get credit for the work. If he doesn't merge and fix up #874 soon, then this is available.

Closes #874
Closes #339

ilanbiala · 2016-01-01T22:47:23Z

@trainerbill are we using this PR or #874?

mleanos · 2016-01-01T23:40:49Z

modules/articles/client/services/models/article.client.model.js

@@ -0,0 +1,19 @@
+(function () {


@rhutchison We don't need this. It's just a duplication of article.client.service

Ok. I see you removed articles.client.service. However, why add it to a models folder & rename with .model.js?

Also, the name should ArticleService to follow John Papa's guide.

Because it's a model, not a service.

Sure, symantically speakijg, it's a model. However, we're treating it as a service; the $resource object is ambigious as to whether it's a model or service. And now this moves a file around, and adds a new directory to our structure.

I just don't see the point in this change if we're not modifying the implementation here. Why even introduce it, just for the sake of making a point that its a model.

I would rather wait, and address client-side models. We'll then be able to take part in a meaningful discussion, rather than tacking on a new idea that isn't relevant to the current PR.

If everyone else is fine with it, then I'll support it. I wanted to at least point it out, and get feedback from others.

@codydaig @ilanbiala @lirantal

I'd say if Angular calls it a service, so should we. Let's not change it just because it matches server-side wording or whatever else cosmetic reason we may have.

In this case, angular calls it a factory, not service. If we are going to have this rigid way of thinking, then menu's should be menuRun?

The style guide leaves this type of detail up to interpretation.

trainerbill · 2016-01-01T23:49:00Z

Close 874. It's messed up and I am out for a few days.

ilanbiala · 2016-01-01T23:59:59Z

modules/articles/client/config/articles.client.routes.js

+        controller: 'ArticlesController',
+        controllerAs: 'vm',
+        resolve: {
+          articleResolve: newArticle


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.

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.

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.

@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

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

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

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

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.

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

Are you sharing the resolved data among multiple child states?

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?

Yep

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.

ilanbiala · 2016-01-02T00:07:44Z

@rhutchison nice progress. Definitely some fixes made that aren't in #874. One thing that I'm not sure about, does it make sense to split the CRUD across multiple controllers? I feel like it segments the code a little.

mleanos · 2016-01-02T00:19:35Z

@rhutchison I've pulled this branch down & tested it. Everything seems to be working as expected. I think we're close on this.

Why are some of the client tests disabled?

Lastly, I don't think we should go with the new models directory, and changing the name of the file to .model.js. I know this isn't truly a service, but rather a model. However, it's still functioning like a service at the moment; nothing has changed so why make this change now? I don't see the point even If it's only to prepare for the use of models on the client-side. As well, I don't see it making anything else any more clear to our users.

rhutchison · 2016-01-02T02:14:36Z

One thing that I'm not sure about, does it make sense to split the CRUD across multiple controllers? I feel like it segments the code a little.

@ilanbiala If you are referring to ListController vs Controller, I would say that it absolutely makes sense for lists vs model. If you just look at the controllers, you will notice there is no overlap - a lot of extra bindings. When the list controller gets more functionality later, you will continue to see they are unrelated (bulk functionality, sorting, filters, etc.)

rhutchison · 2016-01-02T02:46:57Z

Why are some of the client tests disabled?

@mleanos I fixed 2 of the 3. The 3rd I am not exactly sure how to implement atm, so I'll leave it there as a reminder/todo.

@trainerbill

Update the articles module to implement the style guidelines. Much of this work is from @trainerbill Closes meanjs#874 Closes meanjs#339

mleanos · 2016-01-02T07:31:35Z

Tested, and this LGTM. Thank you Ryan!!

codydaig · 2016-01-03T21:38:46Z

Made a suggestion to add a comment where there seems to be a point of confusion for people. Overall, LGTM. Nice work!

ilanbiala · 2016-01-14T03:43:29Z

@lirantal LGTY?

mleanos · 2016-01-15T20:58:23Z

@codydaig @ilanbiala Can we get this merged? It's been sitting for almost two weeks.

I don't think there's anything else up for debate here. Cody's suggestion for a comment for clarification isn't that vital; and I think users will be able to figure it out by just looking at the route configs, even though it's not really clear from just the controller.

I don't mean to be pushy, but we're so close to the finish line with the style-guide. Also, I've been putting off a PR while waiting for this.

codydaig · 2016-01-15T22:29:24Z

@mleanos @lirantal @ilanbiala I'm cool with getting this merged. I think @ilanbiala is on board as well, just waiting final approval from @lirantal.

ilanbiala · 2016-01-15T22:37:36Z

@codydaig let's just merge this and if @lirantal has suggestions, we'll do a new PR.

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

rhutchison force-pushed the article-styleguide branch 2 times, most recently from d7c4ebb to 5fc33c5 Compare January 1, 2016 20:30

rhutchison closed this Jan 1, 2016

rhutchison reopened this Jan 1, 2016

mleanos reviewed Jan 1, 2016
View reviewed changes

ilanbiala reviewed Jan 1, 2016
View reviewed changes

rhutchison force-pushed the article-styleguide branch from 5fc33c5 to 567cdd3 Compare January 2, 2016 02:45

rhutchison force-pushed the article-styleguide branch 2 times, most recently from 9b95a2d to 7677ff6 Compare January 2, 2016 06:43

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

b3ad56e

Update the articles module to implement the style guidelines. Much of this work is from @trainerbill Closes meanjs#874 Closes meanjs#339

rhutchison force-pushed the article-styleguide branch from 7677ff6 to b3ad56e Compare January 2, 2016 06:54

codydaig added the enhance:feature label Jan 3, 2016

codydaig added this to the 0.5.0 milestone Jan 3, 2016

ilanbiala assigned codydaig Jan 15, 2016

codydaig added a commit that referenced this pull request Jan 15, 2016

Merge pull request #1126 from Gym/article-styleguide

6c5b630

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

codydaig merged commit 6c5b630 into meanjs:master Jan 15, 2016

rhutchison mentioned this pull request Feb 11, 2016

Non-conventional use of ng-init #325

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rhutchison commented Jan 1, 2016

ilanbiala commented Jan 1, 2016

mleanos Jan 1, 2016

mleanos Jan 1, 2016

rhutchison Jan 2, 2016

mleanos Jan 2, 2016

ilanbiala Jan 2, 2016

rhutchison Jan 2, 2016

trainerbill commented Jan 1, 2016

ilanbiala Jan 1, 2016

mleanos Jan 2, 2016

rhutchison Jan 2, 2016

ilanbiala Jan 2, 2016

rhutchison Jan 2, 2016

codydaig Jan 3, 2016

ilanbiala Jan 3, 2016

trainerbill Jan 4, 2016

mleanos Jan 6, 2016

trainerbill Jan 6, 2016

ilanbiala commented Jan 2, 2016

mleanos commented Jan 2, 2016

rhutchison commented Jan 2, 2016

rhutchison commented Jan 2, 2016

mleanos commented Jan 2, 2016

codydaig commented Jan 3, 2016

ilanbiala commented Jan 14, 2016

mleanos commented Jan 15, 2016

codydaig commented Jan 15, 2016

ilanbiala commented Jan 15, 2016

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

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

Conversation

rhutchison commented Jan 1, 2016

ilanbiala commented Jan 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trainerbill commented Jan 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilanbiala commented Jan 2, 2016

mleanos commented Jan 2, 2016

rhutchison commented Jan 2, 2016

rhutchison commented Jan 2, 2016

mleanos commented Jan 2, 2016

codydaig commented Jan 3, 2016

ilanbiala commented Jan 14, 2016

mleanos commented Jan 15, 2016

codydaig commented Jan 15, 2016

ilanbiala commented Jan 15, 2016