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

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

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

Conversation

vstene
Copy link

@vstene vstene commented Feb 15, 2016

When doing a page change in ui-router, the scope.on('$destroy') is called after the new view title directive has been initialized, resulting that there is no view title present.

});

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

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

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.

@apparentlymart
Copy link
Owner

Thanks for this! It's a great improvement to the robustness of the title handling.

I just had a design-ish question inline. If you don't have time to work on this further then no worries... just thinking about what is the best way to represent this internal state.

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.

2 participants