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

Call scrollBehavior with app context #1804

Merged
merged 3 commits into from
Oct 23, 2017
Merged

Call scrollBehavior with app context #1804

merged 3 commits into from
Oct 23, 2017

Conversation

homerjam
Copy link
Contributor

This allows us to listen for events in the scroll behavior hook, thus enabling us to resolve a promise returned by the hook upon triggering of some custom event (for example one emited on the $root by an after-leave transition event).

// in scrollBehavior
      this.$root.$once('triggerScroll', () => {
        resolve(position)
      })

// in component

  methods: {
    afterLeave () {
      this.$root.$emit('triggerScroll')
    }
  }
<!-- in template -->
      <transition name="fade" mode="out-in" @after-leave="afterLeave">
        <router-view class="view"></router-view>
      </transition>

Sorry for hastily done PR.

Ref: nuxt/nuxt#1376 (comment)

This allows us to listen for events in the scroll behavior hook, thus enabling us to resolve a promise returned by the hook upon triggering of some custom event (for example one emited on the $root by an after-leave transition event).
@homerjam
Copy link
Contributor Author

Hey @yyx990803 any chance of squeezing this one in with the next patch release? 😃

@yyx990803
Copy link
Member

Maybe passing the instance in as the 4th argument is better.

@homerjam
Copy link
Contributor Author

I was wondering if app was the right context, I kind of followed nuxt conventions as that is how I've entered Vue land. Should I update the PR then?

If you don't mind me asking - what makes you think 4th arg is better?

Thanks!

@posva
Copy link
Member

posva commented Oct 15, 2017

No other functions in vue-router have the context set to the root, and it's a function declared on the router options so it wouldn't be intuitive to have the context set to the app. I also think an extra argument makes more sense

@homerjam
Copy link
Contributor Author

homerjam commented Oct 15, 2017 via email

@homerjam
Copy link
Contributor Author

homerjam commented Oct 18, 2017

@posva @yyx990803 agreed that app doesn't make sense for the context, it should be router if anything.

Why is 4th argument better? I would expect the context to be set in router function/hooks etc. and it seems strange to pass router object to a function of itself.

@homerjam
Copy link
Contributor Author

Happy to change this guys, just let me know.

@yyx990803
Copy link
Member

this pointing to the router instance seems to make sense.

@yyx990803 yyx990803 merged commit cc93169 into vuejs:dev Oct 23, 2017
yyx990803 pushed a commit that referenced this pull request Oct 23, 2017
* Call scrollBehavior with app context

This allows us to listen for events in the scroll behavior hook, thus enabling us to resolve a promise returned by the hook upon triggering of some custom event (for example one emited on the $root by an after-leave transition event).

* Update scrollBehavior example

* change scrollBehavior context to router
@zamakkat
Copy link

zamakkat commented Aug 3, 2018

Thank you so much for this fix it's a life saver! The only thing that worked flawlessly with Vue transitions and vue-router to make silky smooth fades from 1 page to another.

This fix has been around for quite some time without being released, wondering if there are plans for a new release of vue-router soon? It was quite confusing because there was an example in the live documentation but it didn't work in code because the latest official release is 3.0.1 from Oct 14, 2017 (almost 10 months ago!). I am currently resorting to use a manual build against the dev branch...

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.

4 participants