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

Use .filename attribute for CSS and JS files for pathto #1503

Closed
wants to merge 1 commit into from

Conversation

humitos
Copy link
Member

@humitos humitos commented Aug 17, 2023

Sphinx 7.2 changed how it handles the CSS and JS files internally and they cannot be treated as string anymore. We need to access to their .filename attribute to get the same behavior.

Closes #1502
Related #1463

Sphinx 7.2 changed how it handles the CSS and JS files internally and they
cannot be treated as string anymore. We need to access to their `.filename`
attribute to get the same behavior.

Closes #1502
@humitos humitos requested a review from a team as a code owner August 17, 2023 11:46
@humitos humitos self-assigned this Aug 17, 2023
@AA-Turner
Copy link
Contributor

@humitos I believe css_tag should cover the use-cases in this theme, is there a reason why you can't use it, so that we might improve it?

A

@AA-Turner
Copy link
Contributor

.filename was added in Sphinx 1.6.: sphinx-doc/sphinx@e6f8cd6

@@ -32,12 +32,12 @@
{%- if css|attr("rel") %}
<link rel="{{ css.rel }}" href="{{ pathto(css.filename, 1) }}" type="text/css"{% if css.title is not none %} title="{{ css.title }}"{% endif %} />
{%- else %}
<link rel="stylesheet" href="{{ pathto(css, 1) }}" type="text/css" />
<link rel="stylesheet" href="{{ pathto(css.filename, 1) }}" type="text/css" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

CI seems stuck, so I can't tell, but is this change compatible with Sphinx 5?

@humitos
Copy link
Member Author

humitos commented Aug 21, 2023

@humitos I believe css_tag should cover the use-cases in this theme, is there a reason why you can't use it, so that we might improve it?

I wasn't aware of it. I understand you refer to this, right?

https://github.com/sphinx-doc/sphinx/blob/6183b6abee561fc71c88fc66416d31a8f8ca009f/sphinx/builders/html/__init__.py#L1057-L1094

I will give it a try.

@agjohnson pinging you in case you know if there is a blocker about using these functions.

@humitos
Copy link
Member Author

humitos commented Aug 21, 2023

If we are OK using css_tag, there are more code we can update. For example:

{%- for css in css_files %}
{%- if css|attr("rel") %}
<link rel="{{ css.rel }}" href="{{ pathto(css.filename, 1) }}" type="text/css"{% if css.title is not none %} title="{{ css.title }}"{% endif %} />
{%- else %}
<link rel="stylesheet" href="{{ pathto(css.filename, 1) }}" type="text/css" />
{%- endif %}
{%- endfor %}
{%- for cssfile in extra_css_files %}
<link rel="stylesheet" href="{{ pathto(cssfile.filename, 1) }}" type="text/css" />
{%- endfor -%}

@agjohnson
Copy link
Collaborator

@agjohnson pinging you in case you know if there is a blocker about using these functions.

Not off the top of my head. If there was a blocker, it would probably be projects with old/outdated configuration.

@humitos
Copy link
Member Author

humitos commented Aug 22, 2023

It seems we already did something similar to this PR in the past: #1101. I think we should take a look at both and merge them into the final PR 😄

@stsewd stsewd deleted the humitos/pathto-css-js-files branch August 29, 2023 14: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.

Incompatibility with Sphinx 7.2
3 participants