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

Fix case when titleToken returns an array of strings #37

Closed
wants to merge 1 commit into from

Conversation

sly7-7
Copy link
Contributor

@sly7-7 sly7-7 commented Mar 1, 2016

No description provided.

@kimroen
Copy link
Owner

kimroen commented Mar 1, 2016

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
Copy link
Contributor Author

sly7-7 commented Mar 1, 2016

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
Copy link
Owner

kimroen commented Mar 1, 2016

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
Copy link
Contributor Author

sly7-7 commented Mar 1, 2016

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

@kimroen
Copy link
Owner

kimroen commented Mar 7, 2016

Thank you very much for noticing and fixing this.

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

I'll release this shortly. Thanks again! 😄

@kimroen kimroen closed this Mar 7, 2016
@kimroen
Copy link
Owner

kimroen commented Mar 7, 2016

Released as v0.3.1!

@sly7-7 sly7-7 deleted the title-tokens-array-mode branch March 8, 2016 08:30
@sly7-7
Copy link
Contributor Author

sly7-7 commented Mar 8, 2016

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

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