Skip to content
This repository has been archived by the owner on Jul 17, 2024. It is now read-only.

Adress comments of PR 183 and 186 #187

Closed
cedricmoullet opened this issue Jul 29, 2013 · 7 comments
Closed

Adress comments of PR 183 and 186 #187

cedricmoullet opened this issue Jul 29, 2013 · 7 comments

Comments

@cedricmoullet
Copy link
Contributor

#183
#186

This was referenced Jul 29, 2013
@gjn
Copy link
Contributor

gjn commented Jul 30, 2013

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).

@elemoine
Copy link
Contributor

@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.

@elemoine
Copy link
Contributor

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.

@gjn
Copy link
Contributor

gjn commented Jul 30, 2013

"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

@elemoine
Copy link
Contributor

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.

@elemoine
Copy link
Contributor

@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.

@gjn
Copy link
Contributor

gjn commented Jul 30, 2013

Thumbs up!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants