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

Add option to include require.js #327

Merged
merged 2 commits into from
Sep 25, 2019
Merged

Add option to include require.js #327

merged 2 commits into from
Sep 25, 2019

Conversation

mgeier
Copy link
Member

@mgeier mgeier commented Sep 22, 2019

No description provided.

@mgeier
Copy link
Member Author

mgeier commented Sep 22, 2019

@manycoding This reverts your PR #302, can you please check if this still works for you?

@manycoding
Copy link
Contributor

Works like a charm.

@mgeier
Copy link
Member Author

mgeier commented Sep 30, 2019

@manycoding Thanks for testing!

@jorisvandenbossche
Copy link

@mgeier Would it be an option to disable this by default?

I know you can now disable it by setting nbsphinx_requirejs_path = '' in conf.py, but including the requirejs breaks websites that use bootstrap, and at least for me, it was not directly easy to find out that it was nbsphinx / injected requirejs that was causing this.

I suppose there will be a way to let requirejs and bootstrap talk nicely together, but not by using it in the default way (including the bootstrap.js in a script tag). So eg websites using sphinx-bootstrap-theme, and both scikit-learn and pandas also recently started to use bootstrap in their development version (it's on the pandas docs that I notices javascript was broken recently, after the nbsphinx update)

@mgeier
Copy link
Member Author

mgeier commented Oct 13, 2019

Would it be an option to disable this by default?

In general, sure.

Currently, it is enabled by default because Plotly plots needed it, see #128 and #302.

Also, it is loaded in the Classic Notebook, in JupyterLab and in nbviewer, so it seemed to me that people might rely on it being available by default.

I suppose there will be a way to let requirejs and bootstrap talk nicely together

That would be nice.

it's on the pandas docs that I notices javascript was broken recently, after the nbsphinx update

Can you please describe the problem?
It would probably be best to open a new issue for that!

There is a version of the nbsphinx docs using the bootstrap theme (https://nbsphinx.readthedocs.io/en/bootstrap-theme/), is the problem visible there, too?

@mgeier
Copy link
Member Author

mgeier commented Oct 16, 2019

I've found the Pandas issues:
pydata/pydata-sphinx-theme#25,
pydata/pydata-sphinx-theme#30.

@jorisvandenbossche
Copy link

jorisvandenbossche commented Oct 16, 2019

@mgeier Thanks for the answer! I don't directly see the problem happening at https://nbsphinx.readthedocs.io/en/bootstrap-theme/, would need to look more into it to understand the difference (but this week on a work week, will try to check next week)

@mgeier
Copy link
Member Author

mgeier commented Oct 16, 2019

I have the gut feeling that it is related to the bootstrap versions.
Bootstrap 3 works, 4 doesn't. Is that possible?

@jorisvandenbossche
Copy link

Ah, possibly yes. In any case we are using Bootstrap 4.

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.

3 participants