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

Make layers config URL template configurable #261

Merged
merged 2 commits into from
Aug 17, 2013
Merged

Conversation

elemoine
Copy link
Contributor

The layers config URL is currently hard-coded in the gaLayers service. This PR makes it configurable at the application level.

Please review.

@elemoine
Copy link
Contributor Author

Do not merge. I need to fix the tests.

@elemoine
Copy link
Contributor Author

Tests are fixed. Ready for review this time.

@elemoine elemoine mentioned this pull request Aug 16, 2013
@gjn
Copy link
Contributor

gjn commented Aug 16, 2013

Strictly speaking, the component (gaLayers service) has to make assumptions about the application (where the template is defined, in our case in the index.html) to be able to create the correct url. But we have this in other places and it can't be avoided (e.g. a directive calling an options.xxx function which is defined in the application controller)

It's not that I would have a better solution in mind currently. So I'm not against merging this because it puts the url definition to the application layer. +1

@elemoine
Copy link
Contributor Author

Strictly speaking, the component (gaLayers service) has to make assumptions about the application (where the template is defined, in our case in the index.html) to be able to create the correct url. But we have this in other places and it can't be avoided (e.g. a directive calling an options.xxx function which is defined in the application controller)

I don't understand this. Could you please be more specific? I actually don't see the directives making such assumptions.

@elemoine
Copy link
Contributor Author

This PR needs rebasing.

@gjn
Copy link
Contributor

gjn commented Aug 16, 2013

Sorry to be unclear:

var url = layersConfigUrlTemplate
             .replace('{Topic}', topic)
              .replace('{Lang}', lang);

The directive has to know what to replace, e.g. {Topic}. As said, I was strictly speaking and being a little pedantic :)

I don't think it's much different than other such cases. It could be viewed as the application satisfying the components interface. In any case, it's a good change!

@elemoine
Copy link
Contributor Author

Ok. Thanks for the explanations. I understand what you mean now.

@elemoine
Copy link
Contributor Author

I've rebased it, but I'd rather merge #126 first.

@elemoine
Copy link
Contributor Author

Now rebased on #262. I also added a commit making the legend URL template configurable as well.

@elemoine
Copy link
Contributor Author

@gjn please review this again, and merge if you're happy with it.

gjn pushed a commit that referenced this pull request Aug 17, 2013
@gjn gjn merged commit 31140d6 into master Aug 17, 2013
@gjn gjn deleted the dev_layersconfigurl branch August 17, 2013 09:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants