Adress comments of PR 183 and 186 about mf-geoadmin3 HOT 7 CLOSED

I like the changes of #PR183 and #PR186. And they apply well to other components as the catalog tree. It didn't feel natural for these directives to listen to topic changes as they should (theoretically) be agnostic about topics, languages, et. al.

I wonder if we could push this even a little further. Angular has a natural way to listen to changes in directives: by using the variables passed into the scope of the directive. So, something like:

scope: {
  backgroundlayers: '=gaBackroundDirectiveLayers'
}

would allow us to react on changes in backgroundlayers of the parent scope (we wouldn't listen on some global message). I'm not saying this is the way to go (I didn't think this through), I'm just thinking about further refinements...and it'll lead us to rethink on how we view application/ga-api/ol-api of our components.

Also, shouldn't we view the gaLayers service as application specific? Another application might inject a different gaLayers service, just implementing the (implied) interface, but getting it's data differently.

All this might clear up once we have our layer management (which I assume will be a service too).

from mf-geoadmin3.

@gjn very quick note to say that I didn't imagine that gaLayers would a service used at the application level. It's a service that stores the layers configuration, and provides layer-related services (e.g. construct ol layer objects) based on that config.

More on your other comments later.

from mf-geoadmin3.

gaLayers is indeed Swisstopo-specific. But I don't think it should be application-specific. The components are Swisstopo-specific, and doing otherwise would make things much more complex I think. We still write reusable components because we want to be able to write new Swisstopo apps based on these components. And writing components/directives also allows for a good architecture of our code. Also, injecting custom services into directives, as you mention it in your comment, is not an easy thing to do, and I haven't evidence that this is a common pattern. That being said, the gaLayers service is used in our components, but it can also be used by the application directly, in controllers (as in the map example). I personally think that the current design is good on that matter.

from mf-geoadmin3.

"The components are Swisstopo-specific, and doing otherwise would make things much more complex I think."

I agree, it would add another level of complexity (more for some directives than for others). Something we probably don't want as of now. I'm just thinking about all those people who'll miss something like geoExt for ol3... :=)

"That being said, the gaLayers service is used in our components, but it can also be used by the application directly, in controllers."

Well, if application code is using components code, it would feel a little awkward...but I do see your point why gaLayers should be a component. That's enough for me. I'll delete ol-api from my mind for now :)

In any case, we need to decide on a case-by-case basis. As it is now, backgroundlayer directive will be usable only once on a page, as it depends on the global gaLayers service (well, it could be used mutliple times, but each would have the same content). As opposed to map directive, which can be re-used because it doesn't depend on a global service.

Thank alot for the feedback!

We should decide on a case-by-case

from mf-geoadmin3.

I'm just thinking about all those people who'll miss something like geoExt for ol3... :=)

Angular is just one of many common js frameworks. That's one of the reasons I think implementing an Angular/ol3 framework doesn't make too much sense. Again, that doesn't mean we should not write reusable components. I just think we cannot target something as generic as GeoExt.

Well, if application code is using components code, it would feel a little awkward.

I think I disagree with that. I think it's good to developed services meant to be used by component directives that can still be used by applications directly. This is a good sign of flexibility.

As it is now, backgroundlayer directive will be usable only once on a page, as it depends on the global gaLayers service (well, it could be used mutliple times, but each would have the same content).

Yep, good comment. I think that's ok to assume that apps based on our set of components will always work with one set of layers. Similarly we assume that topics are global to the application – the topic directive indeed broadcasts a gaTopicChange event on the root scope when switching topics. And here too I think that's an assumption we can make.

As opposed to map directive, which can be re-used because it doesn't depend on a global service.

You're right. If we wanted that our directives be "pure" they should indeed only read from and write to local scopes, as opposed to broadcasting to and listening on the $rootScope. But again, I think the assumptions we make on the "globality" of layers and topics are ok to me.

We should decide on a case-by-case

Agreed.

Good discussion. Thanks.

from mf-geoadmin3.

@gjn, note that I'm happy to discuss alternatives to the current design, where directives would only read from, and write to, local (isolate and parent) scopes. Going that path may require more code at the application level (in controllers) for wiring things together. And for now I'm not convinced we want that.

from mf-geoadmin3.

Thumbs up!

from mf-geoadmin3.