Recompute href based on router.currentState #132
Conversation
addon/helpers/href-to.js
Outdated
| } | ||
| }, | ||
|
|
||
| routerCurrentStateChange: observer('router.currentState', function() { |
There was a problem hiding this comment.
As we already access the "service:-routing" in hrefTo, perhaps we might be able to use something in it? If so, we could avoid another lookup and use a higher level primitive. Perhaps https://api.emberjs.com/ember/release/classes/RouterService/events/routeDidChange might be useful?
There was a problem hiding this comment.
I initially tried using the event but the version of ember that the add on is using doesn't have the router service events.
There was a problem hiding this comment.
oh yeah, we're currently running "ember-source": "~3.4.8" which is quite old. We should likely upgrade and support whatever version introduced the routing service
There was a problem hiding this comment.
You want that as part of this PR? I can add it.
There was a problem hiding this comment.
Nice one, thanks for offering. Perhaps in another PR which we can land first before this one?
There was a problem hiding this comment.
Ok. Just a heads up I'm not going to get to this till probably Monday next week.
There was a problem hiding this comment.
Also as part of the update, want me to refactor using Octane patterns?
There was a problem hiding this comment.
here is the update PR #133
There was a problem hiding this comment.
rebased with update and added looking at the router routeDidChange event. I still needed a separate getter for the router service vs the routing service. the former has the event handling and the latter has the generateURL method.
There was a problem hiding this comment.
fyi, 3.6 seems to have introduced routeDidChange
| }); | ||
|
|
||
| await settled(); | ||
| assert.equal($('#link-to-links a:contains(About)').attr('href'), '/about?section=two') |
There was a problem hiding this comment.
perhaps it would be useful to also test the behaviour of <LinkTo> alongside {{href-to}} to ensure that they have the same behaviour?
There was a problem hiding this comment.
Yeah I agree and will add once I iterate post Ember update.
|
@GavinJoyce this should be good to go now |
This closes #131