Skip to content
This repository was archived by the owner on Jan 10, 2025. It is now read-only.

Make sure we only remove view title from the directive that set it #14

Open
vstene wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Make sure we only remove view title from the directive that set it #14

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c029e4
Increase readability
Feb 15, 2016
533a18b
If the view title has been updated by another view title directive, d…
Feb 15, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 58 additions & 68 deletions angularjs-viewhead.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,76 +1,66 @@

(function (angular) {
'use strict';

var mod = angular.module('viewhead', []);

// Data service that is shared between all directives
mod.factory('viewTitleDataService', function () {
return {};
});

mod.directive('viewTitle', ['$rootScope', 'viewTitleDataService', function ($rootScope, viewTitleDataService) {
return {
Copy link
Owner

@apparentlymart apparentlymart Feb 19, 2016

Choose a reason for hiding this comment

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

Rather than having the separate viewTitleDataService service, could we instead just have a local variable here inside this factory function, which would then be part of the closure for the link function and shared between all instances? I'm actually a bit rusty on Angular since I've not used it in six months or so, but IIRC this factory function is called once per injector, meaning that any local variables it creates should behave effectively as singletons within a given injector.

This is a relatively minor thing, but this way we'd prevent other directives and services from accessing this private data structure, I think is worth doing if it's easy.

Copy link
Author

@vstene vstene Mar 1, 2016

Choose a reason for hiding this comment

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

I was avoiding the local variable on purpose. Ideally the two directives should be splitted into two files and be merged during a build process. In this case you'll have no access to a local variable. But yes, having a publicly accessible service is also not ideal.

restrict: 'EA',
link: function (scope, iElement) {
// If we've been inserted as an element then we detach from the DOM because the caller
// doesn't want us to have any visual impact in the document.
// Otherwise, we're piggy-backing on an existing element so we'll just leave it alone.
var tagName = iElement[0].tagName.toLowerCase();

if (tagName === 'view-title' || tagName === 'viewtitle') {
iElement.remove();
}

var mod = angular.module('viewhead', []);
scope.$watch(function () {
return iElement.text();
}, function (newTitle) {
viewTitleDataService.id = scope.$id;

var title;
$rootScope.viewTitle = newTitle;
});

mod.directive(
'viewTitle',
['$rootScope', '$timeout', function ($rootScope, $timeout) {
return {
restrict: 'EA',
link: function (scope, iElement, iAttrs, controller, transcludeFn) {
// If we've been inserted as an element then we detach from the DOM because the caller
// doesn't want us to have any visual impact in the document.
// Otherwise, we're piggy-backing on an existing element so we'll just leave it alone.
var tagName = iElement[0].tagName.toLowerCase();
if (tagName === 'view-title' || tagName === 'viewtitle') {
iElement.remove();
}
scope.$on('$destroy', function () {
// Only remove view title if the current title has been set by this scope
if (viewTitleDataService.id === scope.$id) {
delete $rootScope.viewTitle;

scope.$watch(
function () {
return iElement.text();
},
function (newTitle) {
$rootScope.viewTitle = title = newTitle;
}
);
scope.$on(
'$destroy',
function () {
title = undefined;
// Wait until next digest cycle do delete viewTitle
$timeout(function() {
if(!title) {
// No other view-title has reassigned title.
delete $rootScope.viewTitle;
}
});
}
);
}
};
}]
);
delete viewTitleDataService.id;
}
});
}
};
}]);

mod.directive(
'viewHead',
['$document', function ($document) {
var head = angular.element($document[0].head);
return {
restrict: 'A',
link: function (scope, iElement, iAttrs, controller, transcludeFn) {
// Move the element into the head of the document.
// Although the physical location of the document changes, the element remains
// bound to the scope in which it was declared, so it can refer to variables from
// the view scope if necessary.
head.append(iElement);
mod.directive('viewHead', ['$document', function ($document) {
var head = angular.element($document[0].head);

// When the scope is destroyed, remove the element.
// This is on the assumption that we're being used in some sort of view scope.
// It doesn't make sense to use this directive outside of the view, and nor does it
// make sense to use it inside other scope-creating directives like ng-repeat.
scope.$on(
'$destroy',
function () {
iElement.remove();
}
);
}
};
}]
);
return {
restrict: 'A',
link: function (scope, iElement) {
// Move the element into the head of the document.
// Although the physical location of the document changes, the element remains
// bound to the scope in which it was declared, so it can refer to variables from
// the view scope if necessary.
head.append(iElement);

})(angular);
// When the scope is destroyed, remove the element.
// This is on the assumption that we're being used in some sort of view scope.
// It doesn't make sense to use this directive outside of the view, and nor does it
// make sense to use it inside other scope-creating directives like ng-repeat.
scope.$on('$destroy', function () {
iElement.remove();
});
}
};
}]);
})(angular);