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

Plotly #302

Merged
merged 6 commits into from
Jun 25, 2019
Merged

Plotly #302

merged 6 commits into from
Jun 25, 2019

Conversation

manycoding
Copy link
Contributor

Closes #128
@mgeier Please review. I am not sure where to put this javascript in the code. Also, should we include it in all the pages?

@mgeier
Copy link
Member

mgeier commented May 8, 2019

Thanks for this PR!

Also, should we include it in all the pages?

It would probably be nicer to include it only in pages that are generated from notebooks.
It could probably work in a raw:: html directive in RST_TEMPLATE?

Is it really necessary to include a plotly plot in the docs?

What about testing for require.js in the section https://nbsphinx.readthedocs.io/en/0.4.2/code-cells.html#Arbitrary-JavaScript-Output-(HTML-only)?

We could show there that both jQuery and require.js are available.
While you are at it, you could remove the "Note" box, because the jQuery issue has been fixed in the meantime.

mgeier
mgeier reviewed May 8, 2019
View reviewed changes
src/nbsphinx.py Outdated Show resolved Hide resolved
@manycoding
Copy link
Contributor Author

manycoding commented May 8, 2019 •
edited
Loading

@mgeier I put the script in the head, but it doesn't show up. What do I miss?

RST_TEMPLATE = """
{% extends 'rst.tpl' %}

{% block head %}
.. raw:: html

    <script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.1.10/require.min.js"></script>

{% endblock head %}

Is it really necessary to include a plotly plot in the docs?

Plotly is one of major libraries, and I included this to be sure it actually works in this package, since in my experience there's always some package tool where it doesn't work. So, let's actually test it works in all of the packages! I can make a better graph so it looks more funny.

What about testing for require.js in the ...

I am not sure if I am understanding why. For me, as an end-user, plotly example belongs in Plots since as an end-user I don't care about JavaScript, I care that my graphs are saved.

@mgeier
Copy link
Member

mgeier commented May 11, 2019

I put the script in the head, but it doesn't show up. What do I miss?

I guess the block is called header, see https://github.com/jupyter/nbconvert/blob/220596a04bba2e82f98549dcd3bd49dd235c0a28/nbconvert/templates/skeleton/null.tpl#L24-L25.

I am not sure if I am understanding why. For me, as an end-user, plotly example belongs in Plots since as an end-user I don't care about JavaScript, I care that my graphs are saved.

Sure, and I would like to make it easy for Plotly users. Those plots should Just Work.

But I want to keep the dependencies (even if it's only the documentation dependencies) as small as possible, and I don't think we actually need a Plotly plot to test if require.js is available.

@manycoding
Copy link
Contributor Author

manycoding commented May 13, 2019 •
edited
Loading

@mgeier header works, but the script is inserted into the body
Screenshot 2019-05-13 at 11 10 59

I agree upon the dependencies, I removed plotly and graph but how can I test if require.js is in the source?
If I put JavaScript code to check if the script is there, will the script be there during the execution?

@mgeier
Copy link
Member

mgeier commented May 14, 2019

header works, put the script is inserted into the body

Yes, that's expected, because the Notebook only provides a part of the page, the actual HTML <head> element is controlled by Sphinx.

Would you prefer the script to be loaded in <head>?

If yes, I don't know how to do that with Sphinx (other than adding it to all files, not only notebooks).

but how can I test if require.js is in the source?

Good question.

I looked around for a minimal example, but I couldn't find anything concise.

What if we don't change the documentation at all?

After your PR it should work fine, do we really need additional information in the docs?

There are many things which just work even though they are not mentioned in the docs ...

@manycoding
Copy link
Contributor Author

I am ok with both. Let's merge it and see how it goes.

@manycoding manycoding mentioned this pull request Jun 3, 2019
Closed
@manycoding
Copy link
Contributor Author

@mgeier let's merge it? The tests seem to fail because one website was unavailable.

@mgeier mgeier merged commit 9bcb012 into spatialaudio:master Jun 25, 2019
@mgeier
Copy link
Member

mgeier commented Jun 25, 2019

@manycoding Sorry for the delay, I've just merged it.

I would like to make the URL configurable at some point, but for now the fixed URL has to be enough.

@manycoding manycoding deleted the plotly branch June 25, 2019 15:19
@astrojuanlu astrojuanlu mentioned this pull request Jun 25, 2019
Closed
@mgeier mgeier mentioned this pull request Sep 22, 2019
Merged
@mgeier
Copy link
Member

mgeier commented Sep 22, 2019

I've created #327 to make the URL configurable (and optional).

@mgeier mgeier mentioned this pull request Sep 22, 2019
Closed
@phockett phockett mentioned this pull request Apr 7, 2022
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgeier mgeier mgeier 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.

Plotly graphs not showing in documentation
2 participants
@manycoding @mgeier