Allow require.js by bundling all js dependencies UMD style #167
Conversation
hoetmaaiers
changed the title
|
Trying this out by checking the preview:
|
Known bug in 3.5.0: twbs/bootstrap#30553
|
This was a bug related to jquery version 3.5.0. Downgrading to 3.4.1 fixes this. I will provide a new PR fixing this (to keep'm small 😉 ) |
|
I see the same behaviour in the preview here, as on the latest deployed site (https://pydata-sphinx-theme.readthedocs.io/en/latest/), so I suppose it is not caused by this PR. Can you merge in latest master into this branch, there are some conflicts now |
docs/conf.py
Outdated
| @@ -36,6 +36,7 @@ | |||
| "sphinx.ext.autosummary", | |||
| "numpydoc", | |||
| "recommonmark", | |||
| "jupyter_sphinx.execute" | |||
There was a problem hiding this comment.
| "jupyter_sphinx.execute" | |
| "jupyter_sphinx.execute", |
|
Bug is fixed. Conflict is merged. |
|
Thanks! |
pydata_sphinx_theme/layout.html
Outdated
| {% if theme_require_js!=False %} | ||
| <!-- Put RequireJS after bootstrap to avoid clashes with anonymous functions | ||
| see https://stackoverflow.com/questions/15371918/mismatched-anonymous-define-module/23467090#23467090 --> | ||
| <!-- <script src="https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.6/require.min.js"></script> --> |
There was a problem hiding this comment.
shouldn't we just delete all of this?
There was a problem hiding this comment.
yes, cleaning commit coming up
pydata_sphinx_theme/theme.conf
Outdated
| @@ -13,4 +13,4 @@ google_analytics_id = | |||
| show_prev_next = True | |||
| search_bar_text = Search the docs ... | |||
| search_bar_position = sidebar | |||
| navigation_with_keys = True | |||
| require_js = True | |||
There was a problem hiding this comment.
it seems like this is going to clobber the navigation_with_keys option which I don't think we want. Also the require_js option shouldn't be needed anymore right?
There was a problem hiding this comment.
correct, cleaning commit coming up
|
It seems like the requirejs is working just fine in the artifacts from this PR, so other than my two comments I think I'm +1 on this! (though maybe double-check the diff because it seems like there were some unintentional changes in this PR) |
There are only some changes from your commit left, which I think were awaiting removal/clean-up (also the docs need to be reworded or removed) until your confirmation this is working |
|
I cleaned this PR. The All extensions depending on require.js should work plug-and-play without specifying specific require.js related things? |
|
Indeed, this doc section should indeed no longer be needed, as it should work out of the box now. So let's try that out ;) Thanks a lot @hoetmaaiers for this solution! |
|
fantastic - thanks @hoetmaaiers for the fix. I wonder - @jorisvandenbossche how do you feel about me cutting a patch release to see if the github action works, and then I can test out whether this indeed fixes the problems with requirejs in the sphinx_book_theme as well? |
|
@choldgraf that sounds perfect! |
This implements javascript dependencies (Bootstrap) via a Webpack bundle.
This should solve require.js issues.
@choldgraf , could you verify this solves the require js issue you tried to solve in #149. If it does, this PR maybe needs an extra cleaning commit regarding the requires changes.