Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding language change broadcast #174

Merged
merged 1 commit into from
Jul 27, 2013
Merged

Conversation

gjn
Copy link
Contributor

@gjn gjn commented Jul 26, 2013

This adresses #173

The Language directive now broadcasts a gaLanguageChange message
that other controllers/directives can listen to. This allows
directive that rely on outside source which are language
dependant to reload their resources. This is needed because
our services deliver translated things (like catalog entries,
layer names, etc) which are not translated on the client.

The only directive now depending on this is the
backgroundlayerselector. Before this, the language in the combobox
didn't change. Now it does.

The Language directive now broadcasts a gaLanguageChange message
that other controllers/directives can listen to. This allows
directive that rely on outside source which are language
dependant to reload their resources. This is needed because
our services deliver translated things (like catalog entries,
layer names, etc) which are not translated on the client.

The only directive now depending on this is the
backgroundlayerselector. Before this, the language in the combobox
didn't change. Now it does.
cedricmoullet added a commit that referenced this pull request Jul 27, 2013
@cedricmoullet cedricmoullet merged commit fd17560 into master Jul 27, 2013
@cedricmoullet cedricmoullet deleted the dev_language_change branch July 27, 2013 03:48
@elemoine
Copy link
Contributor

The $translate service already broadcasts translationChange events when switching languages. I think we should use that instead of having our gaTranslationSelector directive broadcast a custom event.

Also, it is to be noted that events broadcasted on the $rootScope are by definition very global. So we need to make sure that they actually correspond to global, application-level, state changes.


scope.$on('gaLanguageChange', function() {
gaLayers.reloadTopic();
onTriggeredChange();
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the background layer directive now listens to gaTopicChange and gaLanguageChange events. This sounds like too much responsibility on the background layer directive (and other directives that will rely on the gaLayers service in the future). It does not make sense to me that the background layer directive must know that the layers must be loaded again when the language changes. That's knowledge the background layer directive need not have.

And the reloadTopic function looks hackish to me. Also, I don't think that its name actually captures the action it performs. The gaLayers service doesn't load topics, it loads layers.

Anyway, that suggests to me that a better design should be found.

Here's something I'd try: the gaLayers service itself, instead of the background layer directive, listens to gaTopicChange and gaLanguageChange events. And when such an event occurs the gaLayers service naturally loads new layers from the layersconfig service. And one new layers have been loaded it broadcasts a gaLayersChange event. Directives like the background layer directive just listen to gaLayersChange events. So they no longer need to know that the topic or language has changed, they just react on "layers changed" events, which is the only thing they need to know really. And with that design gaLayers.getBackgroundLayers and friends can even directly return the expected data directly instead of a promise.

This obviously would need to be implemented to know if that actually works.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Eric's point of view.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Le 28 juil. 2013 16:24, "Cédric Moullet" notifications@github.com a
écrit :

In
src/components/backgroundlayerselector/BackgroundLayerSelectorDirective.js:

  •         scope.$on('gaLanguageChange', function() {
    
  •           gaLayers.reloadTopic();
    
  •           onTriggeredChange();
          });
    

I agree with Eric's point of view.


Reply to this email directly or view it on GitHubhttps://github.com//pull/174/files#r5440114
.

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

Successfully merging this pull request may close these issues.

None yet

4 participants