Skip to content

Phase 2 of public Router service - adding replaceWith#14861

Merged
rwjblue merged 1 commit intoemberjs:masterfrom
scalvert:router-service
Jan 22, 2017
Merged

Phase 2 of public Router service - adding replaceWith#14861
rwjblue merged 1 commit intoemberjs:masterfrom
scalvert:router-service

Conversation

@scalvert
Copy link
Contributor

This PR represents the second phase of the public router service described in this RFC.

It extends the API with:

  • replaceWith

Status: Work in progress

Reviewers: @locks @rwjblue @ef4

Changes:

  • Added replaceWith method in public router service
  • Added tests

How to test drive this PR:

@rwjblue
Copy link
Member

rwjblue commented Jan 21, 2017

Looks good to me, why is it still WIP (what things are you still looking to add)?

@scalvert
Copy link
Contributor Author

As per our previous discussions, I wasn't sure if we wanted to try to add more to this or whether this was good to go as is. I'd prefer to keep these additions as small as possible so we can iterate faster.

Thoughts?

@rwjblue
Copy link
Member

rwjblue commented Jan 21, 2017

Yep, agreed. Lets keep the PR's small and focused.

Per our chat in slack I think you have a few changes to make (WRT to transitions in the visit.then callback instead of in various afterModel hooks), but this should be good to go once that is done.

@scalvert scalvert changed the title [WIP] - Phase 2 of public Router service - adding replaceWith Phase 2 of public Router service - adding replaceWith Jan 21, 2017
@public
*/
replaceWith() {
this.router.replaceWith(...arguments);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should return here I believe...

@rwjblue rwjblue merged commit 9e24191 into emberjs:master Jan 22, 2017
@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2017

Awesome job, keep it up? 👏

@scalvert scalvert deleted the router-service branch March 3, 2017 03:42
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