Support changing defaults - fixes issue #15

[xon88]

No description provided.

[vinaygopinath]

Congratulations on creating your first PR :) A few points:

• setDefaultTag looks quite elaborate. I think it can be simplified, avoiding any customization related to title and titleSuffix.
• I'm not sure I see the need for useSuffixOnlyCustomTitle - To address the scenario described in your comment, wouldn't it be enough to set the default title to an empty string ('') (until it is overridden by the value from your API) and the default titleSuffix to the actual value to be used?
• To avoid "undefined" showing up as the value of the default title or titleSuffix, line 62 (and likewise line 64) can be changed to this

$rootScope.ngMeta.title = angular.isDefined(title) ? title : (defaults.title || '');

[xon88]

Thanks for your feedback!

• I've done the customizations on the assumption that the function might be called after the $routeChangeSuccess / $stateChangeSuccess are fired. I can remove them and simplify it as I think this scenario might be farfetched =)
• I agree with your comment regarding useSuffixOnlyCustomTitle as this is the first thing that came to mind. The only issue I saw was the pipe preceding the suffix appearing when there isn't a custom title. However, I am now thinking that another option would be to succeed the custom titles with the pipe, instead of preceding the default suffix.
• I also set the defaults as empty strings to help me with the customizations in the first point, so if you agree with simplifying it, your solution for the "undefined" problem would be a better option.

Let me know what you think =)

[vinaygopinath]

• I think it's safe to assume that the setDefaultTag would be called after the $routeChangeSuccess/$stateChangeSuccess broadcast. As I said in the issue comment thread, it would be necessary to check if the ngMeta[defaultTag] exists. If it already exists, there's nothing to do (this is the default value, it should not override the one specified by the route). If it doesn't, it should set ngMeta[defaulTag] to defaultValue.
• The need for useSuffixOnlyCustomTitle is an outlier in terms of common ngMeta usage and I don't think it should be in the repo. If you'd like to keep that functionality, I'd suggest retaining this code in your fork and importing that into your app. (If you're using npm, check out the docs on how to do that)
• I'd be happy to have the (defaults.title || ''). I'm sure others have run into the problem of "undefined" showing up in their titles as well.

Thank you for your help. I appreciate it!

[xon88]

Hi again,

• I get your point regarding only setting the ngMeta[defaultTag] if it is undefined, however my only concern were the title & titleSuffix. Since they would already be concatenated into one variable, it's impossible to distinguish between the two if any one of them is specified by the state/route. (For example, if only a 'Hello' suffix is set by the state/route and later the setDefaultTag('titleSuffix','World') is called, the ngMeta[title] would already be defined, and therefore not be updated to 'Hello World'). In light of this, I've thought of creating a currentRouteMeta object which holds the current overriden tags (updated on readRouteMeta and also on setTitle and setTag for when these are called after the route/state change). This made the setDefaultTag much cleaner by simply calling the setTitle or setTag directly and passing the values in the currentRouteMeta - the check for undefined was already implemented in these functions.
  -- Also, the farfetched concern was the possibility of someone setting the defaults using the ngMetaProvider, and then resetting them later using the setDefaultTag. With the above fix, even this concern would be addressed. Apologies if I was unclear in my explanation.
• I've reverted the commit for the useSuffixOnlyCustomTitle and will be using the approach explained above. Or else I might be adding a separator variable to be used between title and suffix only in case both are set. In this case, I'll do it in another branch in this fork and import it into my app. I'm using bower, but I can still do it in the same way as npm.
• I've implemented the "undefined" fix proposed.

Thanks for your time and feedback,
Regards

[vinaygopinath]

HI,

Thanks for your input. It's interesting for me to learn how this library is being used :)

Your solution of simply passing on the responsibility of determining which tag to keep/replace (the existing meta tag, if any, or the newly set default tag) is clever and accurate, but your implementation can be improved. currentRouteMeta seems redundant to me, as it is a copy of $rootScope.ngMeta :)

I think I'll accept your PR and make changes based on that.