-
Notifications
You must be signed in to change notification settings - Fork 331
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
Remove double-linking of theme CSS #598
Conversation
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.
LGTM, removing the duplication makes sense, IMHO.
Do you want to capture that in a new issue? |
@damianavila - EDIT: Actually I'm going to revert that - I realized that there was a reason that we were using WebPack heavily here, which is that Bootstrap needs to load at the end of the page body. So I think this is actually a good reason to use the So I'll add some docs to explain this a bit better but functionally the PR is no different than when you reviewed it |
34a5e14
to
2465a91
Compare
OK I just pushed a new commit that adds a bunch of extra explanation about how these assets are set up and compiled. I also removed a few sections specifically about fonts, because I'm pretty sure we don't vendor any custom fonts anymore, and having that information there was confusing. This shouldn't change the functionality at all though, so I'll leave it for a day or two and merge if nobody objects. |
The codecov isn't happy on this, but I'm not sure why because it seems like we cover all the things we need to here. So I'm planning to merge this soon unless somebody objects given that we have an approval already! |
I will take a look soon... |
@damianavila would you like me to continue holding off on merging this? |
Left some comments. I forgot to re-request the review (since I have approved the first iteration) and then I missed it. |
Co-authored-by: Damian Avila <[email protected]>
thanks for the comments @damianavila - accepted both your changes and answered your question, let me know what you think! |
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.
LGTM, there is some codecov checks not succeeding but I would be OK to ignore them.
Currently, this theme links its CSS file twice:
styles/pydata-sphinx-theme.css
in ourtheme.conf
filewebpack-macros.html
so that we can apply a hash digest to itFor example, here's the HTML on our live landing page:
I don't believe that we can remove the theme.conf version because Sphinx requires a CSS file there. So this just adds some quick code to remove the CSS file from the context['script_files'] list. This will mean that Sphinx doesn't directly link it, and instead we only link it via the webpack-macros.html route.
It also updates our docs a bit on why we did it this way