search

kimroen / ember-cli-document-title

Adding document title behaviour to your ember app
MIT License
215 stars 61 forks source link

Fix case when titleToken returns an array of strings #37

Closed sly7-7 closed 8 years ago

kimroen commented 8 years ago

Hi! Thanks for this. I haven't looked in to it closely, but would you mind explaining what was broken and what you did to fix it? It's not immediately clear to me.

sly7-7 commented 8 years ago

Hi, actually, I saw this line in the inline doc: https://github.com/kimroen/ember-cli-document-title/blob/master/vendor/document-title/document-title.js#L6 and wanted to use this form.

If you remove this line: https://github.com/kimroen/ember-cli-document-title/pull/37/files#diff-e5e9bb873d4752465a401c95cc77154eR46, the modified test https://github.com/kimroen/ember-cli-document-title/pull/37/files#diff-174701ab0773fb40940ce3b912432ec3R65 does not pass. The tokens are simply ignored. I think that doing tokens.unshift.apply(this, titleToken); simply does not add the titleToken to the tokens array, but to the route itself, which I think was not intented.

I hope my explanation is clear enough (not beeing a native english)

kimroen commented 8 years ago

I think that's pretty clear - thank you. Hopefully I'll get the chance to explore this and merge your PR in not too long.

Thanks again!

sly7-7 commented 8 years ago

No problem :) Thank you for the addon, just simple and what I need :)

kimroen commented 8 years ago

Thank you very much for noticing and fixing this.

I've merged your changes as 62873ab6998e8cf03611636f308a86317d42b065, but I reverted your test changes and made some new tests for it in b5ebbf96cfc36a21d2da3c0f3631472c7a4a3842, as I prefer tests to test one thing at a time.

I'll release this shortly. Thanks again! :smile:

kimroen commented 8 years ago

Released as v0.3.1!

sly7-7 commented 8 years ago

Thanks @kimroen :+1: By the way the tests look better now :)