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

MAINT: split bootstrap and theme styling #994

Merged
merged 16 commits into from Oct 20, 2022
Merged

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Oct 7, 2022

Fix #142

I actually need help on this one.

What I did:

  • create 2 entry points for our webpack file so that bootstrap and pydata-sphinx-theme are both in separate files.
  • improve the code of the webpack file
  • remove import to bootstrap in the former "index.js"
  • remove bootstrap code from "index.js"

My problem:

  • do we need all the bootstrap variables in the pydata-sphinx-theme.scss ?
  • why is the bootstrap.css no available in the readthedoc build when it works just fine on my local build ?

If anyone is ready to give me a hand that would be much appreciated

@12rambau 12rambau marked this pull request as ready for review October 7, 2022 16:11
Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a great start to me - I agree it will be easier to reason with the CSS and JS if we separate the two cleanly. One comment and two quick answers to your questions:

do we need all the bootstrap variables in the pydata-sphinx-theme.scss ?

Almost certainly not, but I don't think we need to block this PR on that decision as it might be complex to figure out. I'm fine w/ doing that in a follow-up.

why is the bootstrap.css no available in the readthedoc build when it works just fine on my local build ?

I wonder if it's because we aren't packaging the bootstrap JS and CSS with the theme? I believe we do some configuration of static assets with sphinx-theme-builder here:

additional-compiled-static-assets = [
"webpack-macros.html",
"vendor/",
]

So maybe that needs to be modified to tell it to include the bootstrap JS/CSS as well? (I think it automatically picks up the pydata-sphinx-theme.css file because of the configuration in theme.conf.

One way to test this locally is to make sure you're not installing the theme in editable mode - make sure you're installing the package in the same way any other person would install from pypi...sometimes that helps clarify issues due to packaging mistakes

webpack.config.js Outdated Show resolved Hide resolved
@12rambau
Copy link
Collaborator Author

So maybe that needs to be modified to tell it to include the bootstrap JS/CSS as well? (I think it automatically picks up the pydata-sphinx-theme.css file because of the configuration in theme.conf.

nice catch I needed to add the bootstrap .js and .css

Copy link
Collaborator

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This generally looks pretty good to me! The demo on RTD looks to be styled correctly too. A few minor comments in there. @12rambau anything else you are looking to do here?

@12rambau
Copy link
Collaborator Author

anything else you are looking to do here?

@choldgraf, nope it's waiting to be merged

@choldgraf
Copy link
Collaborator

I think once the merge conflict is fixed we are good to go 👍

@choldgraf choldgraf changed the title FIX: split bootstrap and theme styling MAINT: split bootstrap and theme styling Oct 20, 2022
@choldgraf choldgraf merged commit 5133e4a into pydata:main Oct 20, 2022
@choldgraf
Copy link
Collaborator

woot - many thanks @12rambau :-)

@12rambau 12rambau deleted the bootstrap branch October 20, 2022 09:19
kou pushed a commit to groonga/groonga that referenced this pull request Jan 12, 2023
`bootstrap.js` and `bootstrap.css` are necessary to the latest document
site.
They are copied (moved) into result files of `make update-document` by
adding them into files.am.

The reason why we need to add `bootstrap.js` and `bootstrap.css` is
because `pydata-sphinx-theme` split bootstrap and theme styling.

pydata/pydata-sphinx-theme#994
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.

Split up bootstrap and theme css
2 participants