Skip to content

Conversation

@MartinMalinda
Copy link

So I spent an afternoon creating this: https://github.com/martinmalinda/ember-router-helpers-2. But I was not thorough in checking if something like it exists.. and it does of course :).

So I thought I could at least port most of the missing logic here, starting with the dummy app.

The acceptance test completely fails here because there is no support for (query-params) helper.

I can provide support for those as well (https://github.com/MartinMalinda/ember-router-helpers-2/blob/master/addon/helpers/url-for.js#L5).

I have some other features there, such as empty {{transition-to}} that can get the url from event. But I guess that that is a bit strange usecase?

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2017

Just enabled CI for this repo, would you mind force pushing to trigger a new build?

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2017

Also, the QP support you linked too looks like a good addition (I only have small tweaks/suggestions RE: the implementation).

While digging into the failure here, I ran into the issue that #6 fixes (the test suite kept redirecting me 😡 ).

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2017

Adding support for query-params in #7 (I went a slightly different route from your implementation). There are still a few failing tests in this PR after that lands (around active state).

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2017

Ok, just merged the QP support PR. How are we looking here?

@MartinMalinda
Copy link
Author

@rwjblue I can submit a fix for #4. Then everything should be green here and we can merge?

@MartinMalinda
Copy link
Author

I've updated the acceptance test to be more strict for active classes on links with QP.

e. g. if current URL is route?qp=true link to route?qp=false should not be active.

@rwjblue
Copy link
Member

rwjblue commented Aug 21, 2017

@rwjblue I can submit a fix for #4. Then everything should be green here and we can merge?

Yep, sounds good!

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