fix(core): fix page-title directive

[itelo]

No description provided.

[ilanbiala]

@itelo can you add some tests so we can catch any other possible issues?

[mleanos]

LGTM. +1 on adding some tests.

[itelo]

@ilanbiala i don't have much experience creating tests... but i will invest some time on it (:

[mleanos]

@itelo What do you think about @trainerbill's comments in #1183#1186?

I like the simplicity of his implementation. I don't quite see his points on your directive being too coupled with the Article's module. However, if that is the case then de-coupling is a good idea.

[trainerbill]

@mleanos you linked the wrong PR.

@itelo #1186

To be specific: #1186 (comment)

Even did the test for ya since I am such a nice guy ;)

[mleanos]

Thank you Andrew. On a train atm, and using my mobile. Not the most efficient combo ;)

[itelo]

should I add the test hero or create another PR? And should I put the @trainerbill idea here too?

[ilanbiala]

You can add the test here. Also, feel free to give a shot at @trainerbill's idea.

[codydaig]

@itelo What's the status of this PR?

[itelo]

@codydaig I fix the directive. I started create the test but I'm having some problems with the them...

[mleanos]

@itelo Are you still having problems with the tests? If so, hit me up on Gitter & we can work on it together.

[mleanos]

@itelo Can you rebase this PR? I spent some time with this PR and incorporated the test you shared with me: https://github.com/itelo/mean/blob/pageTitleTestTask/modules/core/tests/client/page-title.client.directive.tests.js

I had the same problem you described with the Article resolve in the article.view route. I'm not quite sure how to solve this problem. I sent you a message on Gitter. Let me know if you'd like to work on this together.

Anyone else have any ideas about the tests for this? @trainerbill ?

[itelo]

@mleanos @trainerbill unfortunately i have no time to finished this test, what is the best option?

[lirantal]

@mleanos will you please wrap up this PR and merge please?

[mleanos]

@lirantal I'll work on getting this PR finalized.

@itelo If I can get a working test implemented for this, I'll submit a PR to your branch. In the meantime, could you rebase this branch from upstream's master?

[mleanos]

@lirantal @itelo I'll go ahead and merge this fix as is, and create a separate PR for adding a test.

@itelo Can you rebase this branch?

[lirantal]

@mleanos I'm pretty sure you can pull @itelo 's changes, rebase it, and push to a new PR where @itelo 's commits will still be credited when merging this PR. If so, just go ahead and do that.

[mleanos]

@lirantal You're right, that would work. I just prefer the original developer to handle these types of things, in case complex conflicts arise. However, in this case the conflict looks to be very simple; I think just the deleted file is causing it.

I was just messaging with @itelo, and I'm going to merge & rebase myself.

[mleanos]

@itelo @lirantal

Closing since #1404 has been merged.