regardscitoyens / the-law-factory

Track the french law-making process
https://www.lafabriquedelaloi.fr
GNU Affero General Public License v3.0
76 stars 15 forks source link

Reorganize main module directives #135

Closed njoyard closed 8 years ago

njoyard commented 8 years ago

This PR aims to get rid of modX naming convention and split the huge directives.js script to something more maintainable.

I'm not pushing that directly to the repo because it's a bit of a heavy change and I'd like others to review it first.

Also, maybe given the number of JS files it would be interesting to start thinking of a build script to assemble/minify them all in prod.

fmassot commented 8 years ago

Great @njoyard !!!!

I would reorganize a little bit more the project. Here is what I would expect for the structure (but this is only my opinion)

+-- README.md
+-- etc (?)
|   +-- apache.conf
+-- app
|   +-- index.html
|   +-- app.js
|   +-- css
|   +-- fonts
|   +-- img
|   +-- modules
     |   +-- about
          |   +-- about.js
          |   +-- about.html
     |   +-- common
          |   +-- api-service.js
          |   +-- main-ctrl.js
          |   +-- utils.js
     |   +-- articles
          |   +-- articles.js
          |   +-- articles.vis.js
          |   +-- articles.html
     |   +-- debats
          |   +-- debats.js
          |   +-- debats.vis.js
          |   +-- debats.html
...

I would also drop the use of angular modules, we don't really have independent modules so I will only have keep one module "thelawfactory".

njoyard commented 8 years ago

@fmassot thanks for your input. Do you know a clean way of defining an angular module split in separate files ?

BTW, I edited your comment so that the tree is more visible.

fmassot commented 8 years ago

For exemple, for the about page, just write:

angular.module('theLawFactory')
    .directive('about', ['$rootScope', function ($rootScope) {
        }]);

instead of

angular.module('theLawFactory.about', [])
    .directive('about', ['$rootScope', function ($rootScope) {
        }]);
fmassot commented 8 years ago

angular.module('theLawFactory').directive('about', ...) will just add the directive in the module theLawFactory

njoyard commented 8 years ago

Additional work done:

Looks mergeable to me, tell me what you think :)

RouxRC commented 8 years ago

Mis à part les quelques remarques je ne vois rien à redire, au contraire, merci beaucoup encore, c'est vraiment plus clean !

À merger rapidement histoire de pas se marcher dessus pour les corrections restantes de refactov2.

Restera effectivement la question du build, comme discuté j'ai une préférence pour script/makefile plutot que gulp etc qui demande des dépendances en plus, et j'aimerais privilégier une méthode qui permettrait de toujours avoir en dev un environnement ne nécessitant pas de rebuild pour tester un changement, du genre un index.html qui repose sur un build et un dev.html qui repose sur tous les js.

L'autre question est celle de merger aussi ou pas les dépendances (angular, d3, ...), j'ai tendance à préférer quand un site affiche clairement ce qu'il utilise dans sa source, après ca peut se compenser avec des comments html dans le

njoyard commented 8 years ago

À merger rapidement histoire de pas se marcher dessus pour les corrections restantes de refactov2.

@fmassot good to merge for you ?

Restera effectivement la question du build, comme discuté j'ai une préférence pour script/makefile plutot que gulp etc qui demande des dépendances en plus, et j'aimerais privilégier une méthode qui permettrait de toujours avoir en dev un environnement ne nécessitant pas de rebuild pour tester un changement, du genre un index.html qui repose sur un build et un dev.html qui repose sur tous les js.

I made a proof of concept with a Makefile, works quite well. And keeps separate dev/prod files. There are questions left to solve, but we'll discuss it in a separate PR.

L'autre question est celle de merger aussi ou pas les dépendances (angular, d3, ...), j'ai tendance à préférer quand un site affiche clairement ce qu'il utilise dans sa source, après ca peut se compenser avec des comments html dans le

I can just leave the original script tags in the HTML and comment them.

dans le

Just to make sure you haven't lost an important remark at the end here :)

RouxRC commented 8 years ago

Agree with all, and nope, didn't lose anything important just a few hours of sleep ;)

fmassot commented 8 years ago

sounds perfect :)

just wondering why you kept theLawFactory and theLawFactory.config :)

njoyard commented 8 years ago

Because we can redefine it in config.js and I'm afraid of weird side effects. Maybe I will get rid of it when I've tested it enough.

njoyard commented 8 years ago

Merged into refactor-v2. PR was closed because I accidentaly deleted my njoyard:refactor-v2-directives branch. Rebasing in a train is not a good idea :)