Adapt angular architecture: gmf.authentication #3216
Conversation
| * @type {angular.Module} | ||
| */ | ||
| gmf.authentication.component = angular.module('gmfAuthentication', [ | ||
| gmf.authentication.service.name |
There was a problem hiding this comment.
should be gmf.authentication.service.module.name (see comment in the service too)
contribs/gmf/src/directives/print.js
Outdated
| @@ -2,7 +2,7 @@ goog.provide('gmf.printComponent'); | |||
|
|
|||
| goog.require('gmf'); | |||
| /** @suppress {extraRequire} */ | |||
| goog.require('gmf.authenticationDirective'); | |||
| goog.require('gmf.authentication.module'); | |||
There was a problem hiding this comment.
Why do we requested the directive ?
So what you have done is correct, but can't we require only the service ?
| @@ -0,0 +1,242 @@ | |||
| goog.provide('gmf.authentication.service'); | |||
There was a problem hiding this comment.
File name must be wrote with a uppercase s : Service.js. Cause it's a java-script object.
(Unlike the component, that is a description. The controller must also have an uppercase but anyway it's always private in it's own file)
Also. provide / require must use this uppercase S.
And also the Object name itself
| }; | ||
|
|
||
| // todo? | ||
| gmf.module.value('gmfUser', { |
There was a problem hiding this comment.
Looks not suitable.
@gberaudo We move that in another file ?
There was a problem hiding this comment.
I do not understand why gmfUser is a standalone value instead of being a property of the authentication service.
To keep things simple I would let the value in this file (but register it into the authentication module).
Alternatives are:
- move it to its dedicated file, with its own Angular module and depend on it everywhere.
- make it a property of the authentication service.
| * | ||
| * @extends {ol.events.EventTarget} | ||
| */ | ||
| gmf.authentication.service = class { |
| @@ -2,7 +2,7 @@ goog.provide('gmf.filterselectorComponent'); | |||
|
|
|||
| goog.require('gmf'); | |||
| /** @suppress {extraRequire} */ | |||
| goog.require('gmf.Authentication'); | |||
| goog.require('gmf.authentication.module'); | |||
There was a problem hiding this comment.
Oh, I see, it's for the gmfUser. Definitely, it should be moved in another file. perhaps it's own file like the https://github.com/camptocamp/ngeo/blob/master/src/map/FeatureOverlay.js Object.
And then we require only the user, not the whole module.
(Also in another files with the same situation)
| * username: (string|undefined) | ||
| * }} | ||
| */ | ||
| gmf.authentication.AuthenticationLoginResponse; |
There was a problem hiding this comment.
Please, move this kind of descriptions in the contribs/gmf/options/gmfx.js file
| /** | ||
| * @enum {string} | ||
| */ | ||
| gmf.authentication.AuthenticationRouteSuffix = { |
There was a problem hiding this comment.
If it's in the gmf.authentification.Service file (and used by it), it should be prefixed like that I think:
gmf.authentication.service.AuthenticationRouteSuffix = {...}
Or even gmf.authentication.service.RouteSuffix = {...}
pfirpfel
force-pushed
the
gmf.authentication
branch
from
4691399
to
57e4dd2
Compare
pfirpfel
changed the title
| goog.provide('gmf.authentication.component'); | ||
|
|
||
| goog.require('gmf'); | ||
| /** @suppress {extraRequire} */ |
There was a problem hiding this comment.
You can remove this suppress line.
| } | ||
| }; | ||
|
|
||
| ol.inherits(gmf.authentication.Service, ol.events.EventTarget); |
|
@ger-benjamin Does it look ok to you now? |
pfirpfel
force-pushed
the
gmf.authentication
branch
from
5c219a9
to
79bc2eb
Compare
No description provided.