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

Force JS execution after page/DOM fully loads #4189

Merged
merged 2 commits into from Apr 20, 2016
Merged

Force JS execution after page/DOM fully loads #4189

merged 2 commits into from Apr 20, 2016

Conversation

seanlaw
Copy link
Contributor

@seanlaw seanlaw commented Apr 19, 2016

All pull requests must have an associated issue in the issue tracker. If there
isn't one, please go open an issue describing the defect, deficiency or desired
feature. You can read more about our issue and PR processes in the
wiki.

@bryevdv
Copy link
Member

bryevdv commented Apr 19, 2016

@bokeh/dev This seems a reasonable and straightforward solution to the problem in the linked issue, but maybe there are other considerations or approaches, comments appreciated.

@mattpap
Copy link
Contributor

mattpap commented Apr 19, 2016

This seems either unnecessary or misplaced, but I need to give this some thought.

@mattpap
Copy link
Contributor

mattpap commented Apr 19, 2016

In any case, this shouldn't use window.onload, but addEventListener instead.

@bryevdv
Copy link
Member

bryevdv commented Apr 19, 2016 via email

@seanlaw
Copy link
Contributor Author

seanlaw commented Apr 19, 2016

Cool. I found (and tested) another solution using addEventListener that simply wraps the script in this way:

document.addEventListener("DOMContentLoaded", function(event) { 
    //DOM is loaded and ready
    //Run Bokeh anonymous function here
});

Bryan, you'll have to guide me through what to do next. Do I simply create a new branch again but with the new fix and submit a PR again? Should I wait for more comments before solving the problem multiple different ways?

@bryevdv
Copy link
Member

bryevdv commented Apr 19, 2016

@seanlaw No, GH is really nice in this regard. If you just add/commit the new changes to this branch on your local checkout, and then push again, this PR will be automatically updated.

@bryevdv
Copy link
Member

bryevdv commented Apr 19, 2016

Also FYI the one test failure is spurious and unrelated.

@seanlaw
Copy link
Contributor Author

seanlaw commented Apr 20, 2016

Made the new change. See if that works or is better?

Note that I indented the existing anonymous function and so it might look like I made a lot of changes but they are only formatting changes.

@bryevdv
Copy link
Member

bryevdv commented Apr 20, 2016

@seanlaw Thanks! Yah I noticed the annoying diff, but I can see it is just the small change. GH diffs are usually fairly good but sometimes they can appear messy under some circumstances.

ping @birdsarah @havocp @mattpap comments on this?

@bryevdv
Copy link
Member

bryevdv commented Apr 20, 2016

Verbal conversation with @birdsarah, we can't think of any reasons not to pursue this approach. @mattpap if you still have concerns please open another issue to discuss them. I will merge this went Travis turns green. Thanks for the contribution @seanlaw !

@bryevdv
Copy link
Member

bryevdv commented Apr 20, 2016

As a last check I built the docs and inspected the gallery, which uses autoload_static throughout, and everything works as expected.

@bryevdv bryevdv merged commit 7115c9c into bokeh:master Apr 20, 2016
@seanlaw
Copy link
Contributor Author

seanlaw commented Apr 20, 2016

Amazing! Thanks for pushing it through. I'm guessing this makes me an intro-level bokeh contributor rather than a bokeh developer? :)

@bryevdv
Copy link
Member

bryevdv commented Apr 29, 2016

Unfortunately I think this broke notebook support (I was not aware that notebook show used this same code path, it did not used to). I will open a new issue to investigate

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

Successfully merging this pull request may close these issues.

Bokeh + Flask Causes Race Condition
3 participants