aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
431 stars 187 forks source link

📖 DOC: Add `versionadded-block` Sphinx directive #6310

Closed edan-bainglass closed 6 months ago

edan-bainglass commented 6 months ago

This PR aims to fix an apparent issue with the built-in versionadded directive. The directive was designed similarly as other versionmodified-classed directives in the sense that no content should be used, even though content is apparently supported. However, when content is added, the following behavior occurs:

image

This PR introduces a new versionadded-block for this purpose. It also synchronizes versionmodified-classed directives to a similar style.

image

Lastly, per discussion with @sphuber, versionadded directives have been removed for versions <=2.0 under a soft agreement that these are no longer needed. However, hard agreement is required to retain this change 🙏

sphuber commented 6 months ago

This PR aims to fix an apparent issue with the built-in versionadded directive. The directive was designed similarly as other versionmodified-classed directives in the sense that no content should be used, even though content is apparently supported. However, when content is added, the following behavior occurs:

What makes you conclude that this behavior is a bug or issue? Looking at the docs of pydata-sphinx-theme this styling is intentional. The version added/modified/removed blocks have a different styling compared to the other admonitions.

Is this really a problem that we have to add our own extension that we now need to maintain?

edan-bainglass commented 6 months ago

@sphuber I believe these directives were not meant to be used as blocks in the way we use other admonitions. They seem to have been intended as tags, i.e. no content. When used as such, all is well. However, when used as blocks, each additional line break in the content adds a shading to the background. This quickly becomes illegible, not to mention the resultant style inconsistency (line-break-dependent shading).

sphuber commented 6 months ago

Ok, fair enough. I do agree that when in block mode, having them look more similar to the other admonitions looks better, especially not having a background color. Could we maybe keep the rest of the styling the same, i.e., keep the color and icon in the header? Essentially we are just making sure that the actual content is separated from the block's header and the block has no background color.

edan-bainglass commented 6 months ago

Can you clarify your motivation for keeping the original style? Mine for the change is consistency and disambiguity, i.e. following the general concept of admonitions, while exhibiting unique color/icon (instead of the color of Tip + icon of Info).

sphuber commented 6 months ago

Can you clarify your motivation for keeping the original style? Mine for the change is consistency and disambiguity, i.e. following the general concept of admonitions, while exhibiting unique color/icon (instead of the color of Tip + icon of Info).

Main reason is that UI/UX typically is hard. The pydata-sphinx-theme is used by many popular projects that are a lot bigger than AiiDA, e.g, numpy, scipy, pandas, jupyter etc. I would like to think they have a good reason for the styling they chose. So my default position would be to only change things when they seem really necessary. You convinced me that having no background color for the block, just as with the admonitions makes sense. But don't really see why the color and icon needs to be changed.

Anyway, it is not that big of a deal. If others think this is fine as well, then we can go ahead with your choice. @unkcpz @mbercx @giovannipizzi thoughts?

edan-bainglass commented 6 months ago

Valid point. Though there, I doubt it is used as a block. I mostly recall new features separated into distinct sections, where a "new in version..." tag below the section header is a fine solution. We do this too, but not consistently. Perhaps this should be the actual change. Rather than introducing a block version of the versionadded directive, perhaps we consider a standard of sectioning new features that require a block to explain. I'm fine either way, as long as we're clear and consistent. I'll let the rest comment.

giovannipizzi commented 6 months ago

No strong feelings, I would err for consistency with other projects (and so the last comment of Edan is also relevant), but I'm fine either way

edan-bainglass commented 6 months ago

I've decided to close this PR. I agree with @sphuber that, as this is not an AiiDA thing, AiiDA should not maintain it. Instead, I will open an issue/PR with pydata-sphinx-theme (will link here once opened). If and when implemented there, we can revisit and consider updating our tags. Note that it is likely I will only make a change with them to allow for non-background content in the relevant tags (no change of color/icon to headers).

For now, I recommend we convert any block use of versionadded to a subsection with a tag to avoid the harsh background styling.

Update

While writing the issue at pydata-sphinx-theme, I was unable to reproduce the gradual darker shading bug I had experienced. As such (in the absence of the bug), I will hold off on a PR with them. I think on our end, we should proceed to avoid large versionadded blocks, instead opting for subsections. However, it is good to know that when a small block is appropriate, despite the background extending from the header to the body, it will not cause readability issues.

sphuber commented 6 months ago

While writing the issue at pydata-sphinx-theme, I was unable to reproduce the gradual darker shading bug I had experienced.

That is weird. I can reproduce it. Could you try with this minimal example:

index.rst

.. versionadded:: 2.5
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

.. versionchanged:: 2.5
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

.. deprecated:: 2.5
    Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.

and

conf.py

master_doc = 'index'
html_theme = 'pydata_sphinx_theme'

When built, that gives me image

The documentation states that the .. versionadded:: tag should not be followed by an empty line, but in my experience that doesn't change anything. It is not until the lorem ipsum content is unindented (with a blank line after the admonition tag) that it is no longer considered to be part of the block.

I have tried and reproduced this behavior both with pydata-sphinx-theme v0.14 and v0.15.2

sphuber commented 6 months ago

But I think this behavior may in fact have been intended and so I doubt that they will change it. However, they might agree that a block-version that resembles the other admonitions in style would be useful for certain cases where the admonition text would contain multiple lines and even code snippets. Maybe they would be willing to add a block version similar to what you proposed in this PR.

edan-bainglass commented 6 months ago

That's not the bug. What I had experienced is that with each additional line break, another layer of background would be added causing the background to gradually darken. It is this behavior that I cannot reproduce.

sphuber commented 6 months ago

That's not the bug. What I had experienced is that with each additional line break, another layer of background would be added causing the background to gradually darken. It is this behavior that I cannot reproduce.

Ah, I see. I hadn't understood that. Is that what you described in the OP of this PR? Or is this a bug you discovered later? Anyway, if there is no problem, then we can leave it as is.

edan-bainglass commented 6 months ago

It was the original bug. I take it I did not make it as clear as I could. In any case, this is no longer an issue. I agree to proceed as is.

However, note that I will shortly open a PR proposing a standard of using these tags with content.