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

fix(ujs) always handle native events, _also_ handle turbolinks if appropriate #183

Closed
wants to merge 1 commit into from
Closed

fix(ujs) always handle native events, _also_ handle turbolinks if appropriate #183

wants to merge 1 commit into from

Conversation

rmosolgo
Copy link
Member

As mentioned in #108 , sometimes a turbolinks app still has native events. So,

  • always handle native events
  • also support turbolinks events

Since the UJS functions are public now, we could put turbolinks UJS in its own file, right? Using turbolinks would be:

//= require react_ujs
//= require react_turbolinks_ujs // or something

That's a backwards incompatibility though, maybe we should wait a bit

@xionon
Copy link
Contributor

xionon commented Feb 23, 2015

That's a backwards incompatibility though, maybe we should wait a bit

I don't know if it would divide well along this line, but maybe you could have react_native_ujs and react_turbolinks_ujs, and have react_ujs just be a manifest that includes both?

@rmosolgo
Copy link
Member Author

oh @xionon good call, that would be a nice way of splitting it. I may try it in a later PR.

@rmosolgo
Copy link
Member Author

just realized this could result in an unexpected double-mount on first page load, thinking...

@rmosolgo
Copy link
Member Author

Ok, took another try at this. It should just listen for native unload even when doing turbolinks events.

@rmosolgo
Copy link
Member Author

rmosolgo commented Sep 4, 2015

we don't do anything on unload anymore

@rmosolgo rmosolgo closed this Sep 4, 2015
@rmosolgo rmosolgo deleted the always-handle-native-events branch September 4, 2015 18:32
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.

3 participants