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 #212 #219

Closed
wants to merge 2 commits into from
Closed

Fixes #212 #219

wants to merge 2 commits into from

Conversation

lamoglia
Copy link

@lamoglia lamoglia commented Sep 19, 2016

The change is to load the charts using $(document).ready instead of window.onload to allow charts to render even with page caching enebled (issue #212)

Some logic related to specific turbolinks vesions was removed, need to confirm if it is ok, I haven't seen any side-effects.

@xiaods
Copy link
Collaborator

xiaods commented Sep 19, 2016

LGTM

@michelson
Copy link
Owner

Hi , thanks for this code contribution @lamoglia
Have you tested this with turbolinks 5 or < 5 ?
I think remove turbolinks or vanilla js is a no go.
I would like to reproduce the issue #212 , in our dummy application , ¿are you using turbolinks in your app?

@lamoglia
Copy link
Author

I've tested with turbolinks (2.5.3).
Why those "page:load" listeners are needed when using turbolinks?
I thought that $(document).ready does that job already.

@michelson
Copy link
Owner

Hi @lamoglia, Those are the turbolink's $(document).ready listeners needed to trigger the callbacks when page load.
This: $(document).on('page:load', ready) is the jQuery version for turbolinks, the analog for $(document).ready(ready).
In Turbolinks 5 by the way, page:load was changed to turbolinks:load, that's why we implement the referer logic and Turbolink version < 5 and >=5 checks.
we've Turbolinks 5 support merged a month ago from PR #214 , so remove it in this release makes no sense.
are you using turbolinks along with page caching ?

regards

@lamoglia
Copy link
Author

Ok, I'll try to update my project's turbolinks to >=5 to see if something goes wrong and report back ASAP

@michelson
Copy link
Owner

ok, note that you can disable turbolinks caching https://github.com/turbolinks/turbolinks#opting-out-of-caching

@dredupuika
Copy link

This is a no go!
The problem is that when rails native view caching is on the first time a chart is rendered it gets wrapped in the onload event. And when turbolinks gets the view it fetches the same html from the cache bypassing encapsulate_js. This way we end up with onload instead of turbolinks:load.

My guess is that on Turbolinks 5 even if it's not an XHR request you should go with turbolinks:load.
Will test this tomorrow.

@dredupuika
Copy link

anyhow this fixes it for turbolinks 5!

@lamoglia lamoglia closed this Nov 8, 2016
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.

4 participants