[WIP] feat (core) UI Router Nested View #1183
Conversation
| templateUrl: 'modules/core/client/views/hero.client.view.html' | ||
| }, | ||
| 'main': { | ||
| controller: 'HomeController', |
There was a problem hiding this comment.
totally want to, but didn't want to introduce it in this PR... If I go that route I would need to do it for all the routes which means I have to rewrite almost the entire user client side I think. Maybe we should introduce that in another PR to get it up to date with our standards?
There was a problem hiding this comment.
@trainerbill I'm pretty sure @rhutchison started that with other PRs, so I don't see a reason why it would be so big in this one.
There was a problem hiding this comment.
@trainerbill We're already using controllerAs in the Articles module. https://github.com/meanjs/mean/blob/master/modules/articles/client/config/articles.client.routes.js#L21
Since the merging of the Styleguide implementation, I believe we want to include the styleguide in all PR's going forward, if it makes sense; i.e. the client-side routing/templates are getting modified.
There was a problem hiding this comment.
Yeah that makes sense and I will make that change for anything that currently has controllerAs. I don't want to do it for anything that does not currently have controllerAs otherwise I will need to stylize the entire controller in this PR as well. Right?
|
@ilanbiala Can I get a SGTM, so I can continue into articles and users? |
|
@trainerbill I agree with @ilanbiala .. Let's not rush things too much. We have a couple big PR's already that are making significant changes. I'm generally fine with these changes as well, but I haven't tested it or thoroughly reviewed the impact. |
|
I would love to nail down one PR at a time but everything is so slow. Do we have any work on stylizing the core and user modules? I don't see any reason that this PR could not go in and stylize later. I will just use controller on things that have not been stylized and controllerAs if they have been. When we stylize the rest of the modules, the routes file will already be done and all we have to do is add controllerAs:'vm' |
|
@trainerbill some modules have already been changed, why would making the style guide changes in the same PR be an issue? |
|
@ilanbiala If I conform to all of the style changes I will have to rewrite the non stylized controllers in this PR as well. I don't think I can do controllerAs vm without updating the controllers. I think it would make sense to do a nested views PR and a stylize PR to make them smaller. All someone would need to do in the routes file in the stylize PR is to add the controllerAs syntax. Or I can do it all in one PR... up to you. I would actually prefer @rhutchison to do the style PR as he is the master, but I could pull it off as well. Let me know. |
| @@ -42,6 +43,8 @@ angular.module('users').controller('AuthenticationController', ['$scope', '$stat | |||
| return false; | |||
| } | |||
|
|
|||
| console.log($scope); | |||
trainerbill
changed the title
|
All. This PR should be in a semi workable state. Can you pull it down and give it a spin? I want to get some feedback before I start fixing tests. |
|
This is a big one. Closing it in the meanwhile. |
|
There was still a lot of WIP code and this was touching on a lot of stuff, so I don't think we would want to pursue any of it right away. |
|
Cool thanks Ilan. |
@ilanbiala @mleanos
The goal here is to make it easier for frameworks to inject their views. So there are 3 main views you can use, hero/main/subnav. Each one is simply a container as the submodule itself will include the classes. For instance you would need class="container" or container-fluid on the submodule view as we should allow the full page. This initial PR just does the core. I will work on articles and users if feedback is positive.
Edit: Also it may seem trivial in the core but once users and articles are done you will see the benefit.
Edit2: forgot to mention one major advantage is that the views become smaller and reusable.