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.

Sign up for GitHub

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

Jump to bottom

Disable Turbolinks support when not supported #650

Merged
merged 1 commit into from
Dec 26, 2016
Merged

Disable Turbolinks support when not supported #650

merged 1 commit into from
Dec 26, 2016

Conversation

ka2n
Copy link
Contributor

@ka2n ka2n commented Dec 20, 2016 •
edited by justin808
Loading

By checking with Turbolinks.supported API, fallback unsupported browser to non Turbolinks mode.

This change is Reviewable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 82.75% when pulling 8112f17 on ka2n:degrade_turbolinks into 93fb10b on shakacode:master.

justin808
justin808 reviewed Dec 22, 2016
View reviewed changes
node_package/src/clientStartup.js
@@ -63,6 +63,10 @@ function turbolinksVersion5() {
return (typeof Turbolinks.controller !== 'undefined');
}

function turbolinksSupported() {
return Turbolinks.supported;
Copy link
Member

Choose a reason for hiding this comment

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

  1. Is this compatible with v2 and v5 turbolinks?
  2. What browsers are not supported?

@ka2n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Is this compatible with v2 and v5 turbolinks?

refer to turbolinks/turbolinks and turbolinks/turbolinks-classic repo and it's README.md,

  1. What browsers are not supported?

A browser that doesn't supporthistory.pushState nor requestAnimationFrame, Turbolinks.supported will return false. e.g. IE9-IE10 😩

In my use case, I don't find suitable polyfills/shims for that browser APIs.
But ReactDOM still supporting that old browsers, so React itself works fine.

@justin808
Copy link
Member

Please rebase your changes on top of my latest commit.

@ka2n
Copy link
Contributor Author

ka2n commented Dec 25, 2016

@justin808 I did.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.286% when pulling 656f806 on ka2n:degrade_turbolinks into 8bcaa34 on shakacode:master.

@justin808
Copy link
Member

Looks good. Please update the CHANGELOG.

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

CHANGELOG.md, line 8 at r1 (raw file):

## [Unreleased]
##### Fixed
- Disable Turbolinks support when not supported.

Please see the other changelog entries and make consistent.

Comments from Reviewable

@justin808
Copy link
Member

Please update to latest on master and let me know when you've updated. I'd like to ship 6.3.3 ASAP. Please do your change log entry under 6.3.3.

@ka2n
Disable Turbolinks support when not supported
a4d162e 
By checking with Turbolinks.supported API, fallback unsupported browser to non Turbolinks mode.
@coveralls
Copy link

coveralls commented Dec 26, 2016 •
edited
Loading

Coverage Status

Coverage remained the same at 99.294% when pulling a4d162e on ka2n:degrade_turbolinks into eb54c08 on shakacode:master.

@ka2n
Copy link
Contributor Author

ka2n commented Dec 26, 2016

@justin808 done! Please check.

@justin808
Copy link
Member

:lgtm:

GREAT JOB! Thanks!

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

Comments from Reviewable

@justin808 justin808 merged commit 1ea99d9 into shakacode:master Dec 26, 2016
@ka2n ka2n deleted the degrade_turbolinks branch December 26, 2016 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justin808 justin808 justin808 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ka2n @coveralls @justin808