jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
191 stars 29 forks source link

content.code.annotate feature not working #170

Closed MiddleMan5 closed 1 year ago

MiddleMan5 commented 1 year ago

What happened:

I'm trying to use the content.code.annotate feature, but I can't get it to work inside of a yaml code block. There is also almost no documentation for getting it to work inside sphinx rst.

I've dug into the issue, and it appears that the element that is being passed into the "findCandidateList" function is the <div class="highlight" />. The function accesses the next sibling element which is expected to be an ordered list element, but the div has an extra parent.

https://github.com/jbms/sphinx-immaterial/blob/a944f278cb7f1066482e59b9a17d3311624cd01e/src/assets/javascripts/components/content/code/_/index.ts#L98

If the code were to access the sibling of the parent of the container I believe this code would work correctly, but as of now it just returns null and exits.

Here's an example of my rendered code block:

image

Environment

Sphinx: v5.1.1 sphinx-immaterial: 0.9.1

2bndy5 commented 1 year ago

I was also curious if this worked. I saw @jbms add it to the list of supported features but never inquired.

The code I used to reproduce ```diff diff --git a/docs/conf.py b/docs/conf.py index 968b097..f81726e 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -127,6 +127,7 @@ html_theme_options = { "search.share", "toc.follow", "toc.sticky", + "content.code.annotate", ], "palette": [ { diff --git a/docs/customization.rst b/docs/customization.rst index cd87b26..a9f6e7f 100644 --- a/docs/customization.rst +++ b/docs/customization.rst @@ -250,6 +250,17 @@ Configuration Options of strings. The following features are supported: - `content.code.annotate `_ + + .. rst-example:: + + .. code-block:: yaml + :class: annotate + + steps: + - uses: actions/checkout@v3 # (1) + + 1. This step will assume the name of the action it ``uses``. + - `header.autohide `_ - `navigation.expand `_ - `navigation.instant `_ ```
2bndy5 commented 1 year ago

Reminder: The literal blocks produced by sphinx have a extra div wrapped around them when compared to literal blocks produced by mkdocs.

sphinx code block

<div class="annotate highlight-yaml notranslate">
    <div class="highlight">
        <pre id="__code_2">
            <span></span>
            <button class="md-clipboard md-icon" title="Copy to clipboard" data-clipboard-target="#__code_2 > code"></button>
            <code></code>
        </pre>
    </div>
</div>
<ol class="arabic simple"><li></li></ol>

mkdocs code block

<div class="highlight">
    <pre id="__code_10">
        <span></span>
        <button class="md-clipboard md-icon" title="Copy to clipboard" data-clipboard-target="#__code_10 > code"></button>
        <code></code>
    </pre>
</div>
<ol hidden=""><li></li></ol>

This might be why findCandidateList() fails to locate the list of annotations in sphinx output.

jbms commented 1 year ago

I'm not sure why I added it to the list of supported features --- I definitely didn't test that it worked. Perhaps I just added it by mistake.

2bndy5 commented 1 year ago

I'm not against a simple solution amended to the JS, but only if it truly is a simple solution.

MiddleMan5 commented 1 year ago

I'm definitely interested in using the feature, we have large yaml file examples in our docs that this is perfect for

jbms commented 1 year ago

Another possibility could be to change code blocks to generate HTML more similar to that generated by mkdocs. Not sure if the extra div is needed for something.

2bndy5 commented 1 year ago

I think sphinx was using the extra div for captions, but we monkeypatched that already. We do already have another issue to monkeypatch literal blocks about placement of line numbers (#158), so maybe we could do that while exploring a solution for this. I thought about monkeypatching the literal blocks right after posting the "amend JS" comment 🤦‍♂️.

2bndy5 commented 1 year ago

If removing the extra div proves problematic, then we might be able to move the list of annotations into the extra div using a directive. But this would deviate too much from what a migrating user might expect.

2bndy5 commented 1 year ago

After giving it more thought, I'm leaning toward a new directive because

  1. Removing the extra div might break compatibility with existing user custom CSS and/or other 3rd party Sphinx extensions. Although, I can't think of any practical examples of a 3rd party Sphinx ext that might expect the extra div wrapper.
  2. The literal blocks don't need to be immediately patched for #158 now that they (Sphinx devs) postponed the deprecation/removal of table style line numbers.
  3. We could use the directive for more than just literal blocks. This may take a little more investigating on the JS side though.

I envision the new directive like so:

.. code-block:: yaml

    name: Bulid CI  # (1)

.. annotations::
    1. The name of the CI workflow

A paragraph with (1) annotations.

.. annotations::
    1. Look ma, more noise!

We wouldn't have to support any :class: or :name: options for the directive since its sole purpose is to inject the list into the parent for the previous node.

jbms commented 1 year ago

Yes, a code-annotations directive seems like the best solution.

2bndy5 commented 1 year ago

I chose the term annotations because we might be able to use the same directive when the more general use of annotations makes its way to public releases (2 tiers/funding goals from now). If your looking for uniqueness, we could go with md-annotations like we use for other directives that port mkdocs-material features to sphinx-immaterial (like content tabs, mermaid graphs, and title-less admonitions).

I think the content.code.annotate feature won't need to be specified because the directive could append the CSS class annotate where applicable (apparently tables aren't supported for sizing reasons). Although, adding it to the features list shouldn't break anything when the we're finished implementing it.

2bndy5 commented 1 year ago

Well, I've got a hacky solution almost ready to push, but I found out that adding the annotate class to the literal_block node doesn't actually affect the highlighted div (generated by the pygments bridge). Currently (in my dev env), it doesn't work when captions are used in the code-block (possibly because the extra div node just for the caption)... It works ok with the feature enabled globally, but the hide comment mechanism in the JS side isn't getting triggered. I'm still exploring this, but my hacky solution is a bit prudent in that it doesn't copy any of the original code from sphinx.writers.html5.HTML5Translator.visit_literal_block().

Here's a tease: image

using this rst src ```rst .. rst-example:: Example of code annotations .. code-block:: python html_theme_options = { "features" : [ "content.code.annotate" # (1) ], } .. code-annotations:: 1. This isn't normally needed. ```
2bndy5 commented 1 year ago

Ok, fixed the captions problem. It also doesn't seem to work with line numbers (yet), even though the HTML structure is the exact same as mkdocs. I just pushed what I have to the code-annotations branch.

2bndy5 commented 1 year ago

@MiddleMan5 I'm sorry, I just realized we closed this out but never published it in a release. Currently, real life matters are taking precedence, so you'll have to use the main branch until next release. I'll update this thread when we push out the next release.

If you have trouble installing from main branch (requires npm), then there's the nightly distribution from testPyPI index: https://test.pypi.org/project/sphinx-immaterial/0.10.0.post1.dev8/

MiddleMan5 commented 1 year ago

@2bndy5 hey thanks for the update, no worries, I noticed that and switched to installing through git and that's been working

2bndy5 commented 1 year ago

Feature is available in v0.11.0