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

Set jquery as external to allow cdn version plugin attachment #179

Conversation

hoetmaaiers
Copy link
Collaborator

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.

@choldgraf
Copy link
Collaborator

Again a long solution search to end up with a pretty neat solution.

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%" 😅

Copy link
Member

@jorisvandenbossche jorisvandenbossche left a 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';
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

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 ?

@hoetmaaiers
Copy link
Collaborator Author

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: $('h1').tooltip({title: 'test'}).

@jorisvandenbossche
Copy link
Member

@hoetmaaiers thanks for the added comments!

@choldgraf
Copy link
Collaborator

The tooltip works in my test! Thanks @hoetmaaiers 🎉

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.

None yet

3 participants