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

Remove use of $translate in ImportKml directive #185

Merged
merged 1 commit into from Jul 29, 2013
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/components/importkml/ImportKmlDirective.js
Expand Up @@ -202,8 +202,8 @@
}]);

module.directive('gaImportKml',
['$http', '$log', '$translate', 'gaBrowserSniffer',
function($http, $log, $translate, gaBrowserSniffer) {
['$http', '$log', '$compile', '$translate', 'gaBrowserSniffer',
function($http, $log, $compile, $translate, gaBrowserSniffer) {
return {
retsrict: 'A',
templateUrl: 'components/importkml/partials/importkml.html',
Expand Down Expand Up @@ -255,9 +255,13 @@
// Register drag'n'drop events on <body>
var dropZone = angular.element(
'<div class="import-kml-drop-zone">' +
' <div>' + $translate('drop_me_here') + '</div>' +
' <div>{{"drop_me_here" | translate}}</div>' +
'</div>');

// We use $compile only for the translation,
// $translate("drop_me_here") didn't work in prod mode
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment should be removed. It is misleading because using the $translate service doesn't work either in dev mode. The directive must be used or the string won't be updated when switching languages.

$compile(dropZone)(scope);

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know the reason why it didn't work in prod?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @gjn. Wanted to ask the same. It doesn't make sense. And it may actually hide a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I didn't find why. I thought it was a goog.require missing or something like that but no. And when I 've tried to debug the code with the chrome debugger it works, maybe a loading problem of json files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Differences between prod and src are usually big smells. If it was a silent failure (without any error in the console), it could mean that $translate is known but not yet initialized with the translations the moment it's called.

Seems like a timing/async issue to me. We probably were just lucky in src mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I correct what I've said:

In prod mode that never works

In dev mode if you clear the cache, sometimes it works sometimes no

Copy link
Contributor

Choose a reason for hiding this comment

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

Now it makes more sense. Probably related to the asynchronous loading of translation files, as @gjn said it. We should look at the problem more closely, to understand when it triggers, otherwise we'll fall into the trap again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I confirm, when you clear the cache the async requests, which load the json files, finish after the link function is executed.

I've added an issue: #198

Copy link
Contributor

Choose a reason for hiding this comment

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

I think $compile is needed whenever there is angular bindings inside the html snippet. So having $compile makes sense.

<div>' + $translate('drop_me_here') + '</div> would not react to language changes.

Edit: I know see why <div>' + $translate('drop_me_here') + '</div> worked. It's calling the function directly, and it was not part of the html snipped.

var dragEnterZone = angular.element(document.body);
dragEnterZone.append(dropZone);
dragEnterZone.bind('dragenter', function(evt) {
Expand Down