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

Remove double-linking of theme CSS #598

Merged
merged 5 commits into from Mar 10, 2022
Merged

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Feb 24, 2022

Currently, this theme links its CSS file twice:

  • Once via the default Sphinx pathway, because we specify styles/pydata-sphinx-theme.css in our theme.conf file
  • Once via webpack-macros.html so that we can apply a hash digest to it

For example, here's the HTML on our live landing page:

chrome_Tm5oppVLZh

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

Copy link
Collaborator

@damianavila damianavila left a 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.

@damianavila
Copy link
Collaborator

Note for future: ultimately, I think we could do the hashing inside of Python, rather than with webpack.

Do you want to capture that in a new issue?

@choldgraf
Copy link
Collaborator Author

choldgraf commented Feb 25, 2022

@damianavila - I actually looked at the code and thought it might be easier to just do this "the right way" and simplify our webpack build process bit. So I've re-worked this PR to do the hashing in Python instead of JavaScript. Check the top comment for extra details.

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 macro + webpack combo here, and for simplicity's sake we might as well do the same with CSS.

So I'll add some docs to explain this a bit better but functionally the PR is no different than when you reviewed it

@choldgraf
Copy link
Collaborator Author

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.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Feb 25, 2022

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!

@damianavila
Copy link
Collaborator

I'll leave it for a day or two and merge if nobody objects.

I will take a look soon...

@choldgraf
Copy link
Collaborator Author

@damianavila would you like me to continue holding off on merging this?

webpack.config.js Outdated Show resolved Hide resolved
@damianavila
Copy link
Collaborator

@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.
Thanks for the ping 😉!

Co-authored-by: Damian Avila <damianavila82@yahoo.com.ar>
@choldgraf
Copy link
Collaborator Author

thanks for the comments @damianavila - accepted both your changes and answered your question, let me know what you think!

Copy link
Collaborator

@damianavila damianavila left a 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.

@choldgraf choldgraf merged commit 3c11db8 into pydata:master Mar 10, 2022
@choldgraf choldgraf deleted the fix-double-css branch March 10, 2022 04:04
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

2 participants