feat: add refresh method #84
Conversation
|
Thank you @aklkv for contribution! Do you think you could add some test case around this new functionality? Just to make sure we don't accidentally regress in future |
|
I tried but failed to run the test-app locally, I was running into couple of issues.
|
| @@ -65,6 +65,24 @@ export default class EngineRouterService extends Service.extend(Evented) { | |||
| return this._externalRoutes[externalRouteName]; | |||
| } | |||
|
|
|||
| refresh(routeName = this.currentRouteName, ...args) { | |||
There was a problem hiding this comment.
this approach might be able to solve #74 as well @SergeAstapov if you want I can add it as well
There was a problem hiding this comment.
@aklkv sure, feel free to open separate PR, to keep each PR focused on one thing
There was a problem hiding this comment.
I agree with @SergeAstapov here, let's open another PR for this purpose
aklkv
requested review from
SergeAstapov and
villander
and removed request for
SergeAstapov and
villander
test-app/tests/acceptance/routeable-engine-demo-refresh-test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Big thanks @aklkv for working on this and bearing with us on the feedback!
this looks good from my side.
|
@SergeAstapov Thank you for taking the time to review these changes! Looking forward to get this out |
|
@villander Please let me know if I can do anything to get this changes in. Thank you! |
|
@SergeAstapov @villander just re-pinging to see if I can do anything else to get this over the finish line |
|
we need @villander to pull the trigger as I don't have permissions |
|
I just ran into this one too, happy to see there's already a PR open 🎉 A most gentle ping for @villander, hopefully this can be merged and a release can get cut 🥺 |
|
@villander is there anything I can do to get this PR moving? |
closes: #81