-
Notifications
You must be signed in to change notification settings - Fork 323
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
Set jquery as external to allow cdn version plugin attachment #179
Set jquery as external to allow cdn version plugin attachment #179
Conversation
Those are the best kinds of problems and solutions :-) I will always take a solution that makes you go "gosh was the problem really that hard if this was the solution?" over a solution that makes you go "holy shit that was such a beast thank god we fixed it by growing our codebase by 30%" 😅 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@hoetmaaiers one request: could you maybe try to add a bit more inline comments on why certain things are done this way (can be similar as what you said in the top post of this PR), just to ensure that in the future if someone wants to understand this, he/she doesn't need a long search again ;)
@choldgraf did you test that this fixes the tooltip issue?
What is the easiest way to add a tooltip to the demo docs so we can test it there as well? (maybe just a raw html div with a small bootstrap snippet with a tooltip?)
import 'popper.js'; | ||
import 'bootstrap'; | ||
|
||
import './../scss/index.scss'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the css needs to be imported here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialy the scss was imported on Webpack level as an entry: https://github.com/pandas-dev/pydata-sphinx-theme/pull/179/files#diff-5c572bb8f24a00a84aa28f22d3a39ba5L7. Best practice in the Webpack community is to have a single javascript entry (or multiple when chunk or split bundling).
The main advantage of only define index.js
as the entry is that Webpack can take care of bundling all other imported assets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I suppose I just don't really understand the relation with the actual layout.html, where we still load both index.js and index.css ?
I've added some documentation, good you thought about this @jorisvandenbossche . To use a tooltip (or test the feature), it's as easy as typing into the console: |
@hoetmaaiers thanks for the added comments! |
The tooltip works in my test! Thanks @hoetmaaiers 🎉 |
This PR notifies Webpack of jQuery already being loaded outside of its world. This way Sphinx related javascript and custom javascript like popper.js can hook into jQuery.
Solves #176.
Again a long solution search to end up with a pretty neat solution.
I also took the liberty to add some more structure to the index.js javascript.