-
Notifications
You must be signed in to change notification settings - Fork 242
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 #223 #224
base: master
Are you sure you want to change the base?
Fixes #223 #224
Conversation
Also fixed Rails caching problems for Turbolinks 5 |
def request_turbolinks_5_tureferrer? | ||
defined?(request) && request.respond_to?(:headers) && request.headers["Turbolinks-Referrer"] | ||
def is_turbolinks_5? | ||
Gem::Version.new(Turbolinks::VERSION) >= Gem::Version.new('5.0.0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. Question: what would happen if you have Turbolinks present in the project but not enabled in javascript ? I think in that case this solution will always return true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't cross my mind. Then i'm at my wit's end with this. I guess there is no way to determine if rails has loaded turbolinks.js with certainty.
My best bet is that it is only fixable by manually passing an argument that switches the turbolinks wrapper on/off
Anyhow. I guess i can't help on this one then.
Will keep using this for my implementation for the time being though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a mix of the two solutions ?
is_turbolinks_5? && request_turbolinks_5_tureferrer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @michelson ... The solution that only checks the gem version causes problems when I'm rendering charts in a PDF, where I have JS enabled but Turbolinks is not enabled (it is on my application though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that has been fixed long ago with this this commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, my charts on PDFs are still not being rendered, don't know why... (they are rendered normally when using @michelsons master).
#{core_js} | ||
e.target.removeEventListener(e.type, arguments.callee); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting, can you explain to us what this code does. where is the arguments
variable is defined ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arguments.callee property contains the currently executing function. doc
you can always use:
var f = function(){ document.removeEventListener('turbolinks:load', f, true); #{core_js} }; document.addEventListener('turbolinks:load', f, true);
does the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance you can merge this fix without the rest of the PR? This fix is really important, but on the other hand other commits from this PR break my code (e.g. verifying how to encapsulate JS only based on the Turbolinks v5 gem being loaded into the application)
Hi @dredupuika , thanks for this PR submission. best |
What if we do it this way? |
@michelson did you have time to check this commit? |
I can confirm this works. @michelson would be nice to see this merged and released so we don't have to use a fork any longer. |
Remove event listener after it is executed