Skip to content

Conversation

@faceyspacey
Copy link
Collaborator

@faceyspacey faceyspacey commented Aug 28, 2017

So the real correct way to do this seems to be to fix this function getHookArgumentsLength, however it seems that it has a hard time analyzing the number of arguments in the vue-to-react transpilation scheme setup here.

I could be wrong, so consider this just an indicator of what needs to be fixed.

Essentially, my fix is a weaker assumption:

  • if there is an onLeave handler, it means do not remove the element.

However, in a real Vue app, you likely can use such handlers for other things without manually removing the element, which is why getHookArgumentsLength exists to smooth things over. My guess is those situations are just edge cases. 90% of the time if you are going to provide such a handler you will finish the job and call done yourself.

Hopefully this can get merged sooner than later, as I'd like to present your repo (since you're the original creator) rather than a fork.

So the real correct way to do this seems to be to fix this function `getHookArgumentsLength`, however it seems that it has a hard time analyzing the number of arguments in the vue-to-react transpilation scheme setup here. 

I could be wrong, so consider this just an indicator of what needs to be fixed. Essentially, my fix is a weaker assumption: *if there is an `onLeave` handler, it means do not remove the element. However, in a real Vue app, you likely can use such handlers for other things without manually removing the element, which is why `getHookArgumentsLength` exists to smooth things over. My guess is those situations are just edge cases. Most of the time if you are going to provide such a handler you will finish the job and call `done` yourself. Hopefully this can get merged sooner than later, as I'd like to present your repo (since you're the original creator) rather than a fork.
@faceyspacey
Copy link
Collaborator Author

I don't suppose there is anyway to get this merged to day, as i gotta submit my presentation today, and it would be best to present your actual packages rather than forks.

@SmallComfort
Copy link
Owner

I'm very grateful to you for your contribution, and I'm sorry not to reply to you in time. I don't have much time to handle issues this week because of my busy work. I promise all of the issues will be resolved in this weekend. I apologize for the inconvenience.

@SmallComfort SmallComfort merged commit 31f50d1 into SmallComfort:dev Aug 29, 2017
@faceyspacey
Copy link
Collaborator Author

faceyspacey commented Aug 29, 2017

It's totally fine. I skipped doing an example of transition-group. So all I needed was <transition>. So with the merge you made, I'm good to go. Thanks again!

I'll send you the full demo when I have permission.

@faceyspacey
Copy link
Collaborator Author

one last thing: do you think you can publish the update for react-vue-helper to npm?

@faceyspacey faceyspacey changed the title do not remove <transition> element if onLeave handler provided does not remove <transition> element if onLeave handler provided Aug 29, 2017
@faceyspacey
Copy link
Collaborator Author

any word?

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