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

Fixes turbolinks loading issue #221

Merged
merged 7 commits into from
Jan 24, 2016
Merged

Fixes turbolinks loading issue #221

merged 7 commits into from
Jan 24, 2016

Conversation

justin808
Copy link
Member

Code was checking that turbolinks was installed when it was not yet
because some setups load turbolinks after the bundles. The changes to
the code will check if turbolinks is installed after the page loaded
event fires.

Code was also added to allow easy debugging of Turbolinks, which should
be useful when v5 of Turbolinks is released shortly.

Details of how to configure Turbolinks with troubleshooting were added
to docs/additional_reading/turbolinks.md.

Also:

  • Changed generation of the examples apps to /tmp/examples. This allows
    one to configure editors to ignore /tmp. Ignoring /examples is
    problematic as one deletes the /examples directory frequently.

Review on Reviewable

Code was checking that turbolinks was installed when it was not yet
because some setups load turbolinks after the bundles. The changes to
the code will check if turbolinks is installed after the page loaded
event fires.

Code was also added to allow easy debugging of Turbolinks, which should
be useful when v5 of Turbolinks is released shortly.

Details of how to configure Turbolinks with troubleshooting were added
to docs/additional_reading/turbolinks.md.

Also:

* Changed generation of the examples apps to /tmp/examples. This allows
  one to configure editors to ignore /tmp. Ignoring /examples is
  problematic as one deletes the /examples directory frequently.
@justin808
Copy link
Member Author

@alexfedoseev Can you please review ASAP


Review status: 0 of 17 files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

@justin808 Looks good! One small comment.


Review status: 0 of 22 files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


spec/dummy/client/app/components/HelloWorld.jsx, line 22 [r1] (raw file):
This is kinda legacy way.

setDomRef(domNode) {
  this.myDomRef = domNode;
}

handleChange() {
  const myDomNode = this.myDomRef;
  // ...
}

render() {
  return (
    <div
      ref={this.setDomRef}
      onChange={this.handleChange}
    />
  );
}

Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 24, 2016

Reviewed 22 of 22 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.


Comments from the review on Reviewable.io

@justin808
Copy link
Member Author

Review status: 22 of 24 files reviewed at latest revision, 1 unresolved discussion.


spec/dummy/client/app/components/HelloWorld.jsx, line 22 [r1] (raw file):
@alexfedoseev Any reference? Why is this better?


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 24, 2016

Review status: 22 of 24 files reviewed at latest revision, 1 unresolved discussion.


spec/dummy/client/app/components/HelloWorld.jsx, line 22 [r1] (raw file):
I still think <input ref={(ref) => this.name = ref} /> is the slickest, but the style guide won't allow it :(


Comments from the review on Reviewable.io

@robwise
Copy link
Contributor

robwise commented Jan 24, 2016

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

@alex35mil
Copy link
Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


spec/dummy/client/app/components/HelloWorld.jsx, line 22 [r1] (raw file):
http://facebook.github.io/react/docs/more-about-refs.html#the-ref-string-attribute


Comments from the review on Reviewable.io

@justin808 justin808 force-pushed the fix-turbolinks-issue branch from 3566a4f to 8b5c154 Compare January 24, 2016 19:24
@robwise
Copy link
Contributor

robwise commented Jan 24, 2016

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

justin808 added a commit that referenced this pull request Jan 24, 2016
@justin808 justin808 merged commit ef9ccbc into master Jan 24, 2016
@justin808
Copy link
Member Author

Woo hoo! releasing later today!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants