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

layout: allow arbitrary CSS attributes #1101

Closed
wants to merge 2 commits into from

Conversation

gmarull
Copy link

@gmarull gmarull commented Mar 25, 2021

Allow arbitrary CSS attributes when using app.add_css_file() on Sphinx.
It can be useful e.g. to insert media or any other attribute. Note
that rel and type are set by default.

Fixes #1100

@Blendify
Copy link
Member

Does feature for other sphinx themes? This is what the basic theme does, does this syntax work?

    {%- for css in css_files %}
      {%- if css|attr("filename") %}
    {{ css_tag(css) }}
      {%- else %}
    <link rel="stylesheet" href="{{ pathto(css, 1)|e }}" type="text/css" />
      {%- endif %}
    {%- endfor %}

@Blendify Blendify requested review from Blendify and a team March 25, 2021 19:42
@Blendify Blendify added this to the 1.1 milestone Mar 25, 2021
@gmarull
Copy link
Author

gmarull commented Mar 25, 2021

@Blendify thanks for pointing that out! I was not aware of css_tag.

@Blendify
Copy link
Member

The change is good, however it requires sphinx 1.8 but currently we support versions down to 1.6 so this will need to be an if else block see https://github.com/readthedocs/sphinx_rtd_theme/blob/master/sphinx_rtd_theme/layout.html#L59 or wait until #1075.

If you use the if else block I will add this to the upcoming 1.0 release.

Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

Meant to request changes

@gmarull gmarull requested review from Blendify and removed request for a team March 26, 2021 14:51
Comment on lines 26 to 28
{%- 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 %}
Copy link
Member

Choose a reason for hiding this comment

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

The logic here is a bit messed up I'm starting to realize it is a bit more complicated than I originally thought.

 <link rel="stylesheet" href="{{ pathto('_static/' + style, 1) }}" type="text/css" />
  <link rel="stylesheet" href="{{ pathto('_static/pygments.css', 1) }}" type="text/css" />
  {%- for css in css_files %}
    {%- if sphinx_version >= "1.8.0" %}
      {%- if css|attr("filename") %}
        {{ css_tag(css) }}
      {%- else %}
         <link rel="stylesheet" href="{{ pathto(css, 1) }}" type="text/css" />
      {%- endif %}
    {%- else %}
      {%- 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" />
      {%- endif %}
    {%- endif %}
  {%- endfor %}

Copy link
Author

Choose a reason for hiding this comment

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

I see filename attribute already present on 1.8.0, so why is the additional check needed?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure but this is what sphinx does: https://github.com/sphinx-doc/sphinx/blob/3.x/sphinx/themes/basic/layout.html#L97

The case for before 1.8 is to match the template before this commit: sphinx-doc/sphinx@c5d6942

Copy link
Author

Choose a reason for hiding this comment

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

ok, should be ready now

Copy link
Author

Choose a reason for hiding this comment

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

@Blendify ping

Allow arbirtrary CSS attributes when using app.add_css_file() on Sphinx.
It can be useful e.g. to insert `media` or any other attribute. Only
supported on Sphinx >= 1.8.0.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Copy link
Member

@Blendify Blendify left a comment

Choose a reason for hiding this comment

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

Looks good but we are going to hold off until 1.1 release.

@Blendify Blendify requested review from agjohnson and stsewd April 6, 2021 23:46
@Blendify
Copy link
Member

Blendify commented Apr 6, 2021

It seems to merge is blocked however, maybe because you force pushed a commit

@benjaoming
Copy link
Contributor

We still support Sphinx 1.6 in 1.1... but will be dropped in 2.0. See: https://sphinx-rtd-theme.readthedocs.io/en/stable/development.html#release-2-0-0

@benjaoming benjaoming modified the milestones: 1.1, 2.0 Aug 27, 2022
@gmarull
Copy link
Author

gmarull commented May 30, 2023

any chance this can be merged? @Blendify

@agjohnson
Copy link
Collaborator

Note, this was merged in #1519

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.

arbitrary add_css_file attributes are not supported
4 participants