Adjust docutils container class addition

[stijnvanhoey]

When using the docutils container directive to add custom css classes to a node, docutils automatically adds the docutiles and container class to the element. As bootstrap itself uses a container class as well, this creates interference.

To avoid this, the PR adjusts the docutils container directive and only uses the css classes provided by the user.

[choldgraf]

Hmmm - does this mean that any other Sphinx extension that alters the container class will then be over-written by this theme?

[jorisvandenbossche]

@choldgraf are you aware of such extensions?

Note that this is "just" the container docutils node. I don't think it is a typical thing to do to overwrite this. But I am not fully sure when this node is used (certainly for the the .. container:: directive).

[jorisvandenbossche]

I checked in the built pandas docs, and there the "docutils container" classes in divs currently only occur in the output from nbsphinx, because they use the container node in the docutils tree they generate from the notebook (https://github.com/spatialaudio/nbsphinx/blob/c74aa47bbddde83a0954c93d5ad65b6ccbd885c4/src/nbsphinx.py#L973)

And they also use this as a selector for styling:

https://github.com/spatialaudio/nbsphinx/blob/c74aa47bbddde83a0954c93d5ad65b6ccbd885c4/src/nbsphinx.py#L504-L510

(and more)

So yes, this patch would break nbsphinx ... (so thanks for asking the question! ;))
(I checked it in the browser, and indeed if you remove the container class from a code cell, eg at https://dev.pandas.io/docs/user_guide/style.html, then the styling is broken)

Personally, I think nbsphinx should rather not use this container css class to style (they already add their own classes, so I don't think they need it). But OK, we will need to find another solution here. We will probably need to add a new docutils node (eg "div"), that does the exact same thing as the container node, except for adding those "docutils container" classes to the div.

[choldgraf]

couldn't this just be something like below? I think the details probably are off, but the general idea?

Suggested change

	self.body.append(self.starttag(node, "div", CLASS=""))
	classes = ' '.join(filter(lambda ii: ii != "container", node.attributes.classes))
	self.body.append(self.starttag(node, "div", CLASS=classes))

[jorisvandenbossche]

Yes, that's indeed more correct (because the whole purpose was to use eg .. container:: some-bootstrap-class, so we indeed need to pass that through as class into the html)

But, that still doesn't solve the issue of removing "container" causing issues for projects that rely on it for theming (like nbsphinx)

[choldgraf]

yeah that makes sense - probably not a good idea to alter fundamental default sphinx behavior

[jorisvandenbossche]

Actually, I was a bit fast with commenting, as there should be some other way that the user-defined classes are pass through, as the base docutils implementation is:

    def visit_container(self, node):
        self.body.append(self.starttag(node, 'div', CLASS='docutils container'))

so we have exactly the same just without the two classes. I suppose the CLASS option above is only additive to the classes that are already defined in the node itself (coming from parsing the .. container:: directive)

[stijnvanhoey]

Indeed, as far as I understood, the other classes are passed as well, this removes only these two. Still, there is probably a better way to do this, but I do not have any other idea for the moment.

[jorisvandenbossche]

So to conclude here: let's make a new directive that is kind of a copy-paste of the container directive, but with another name ("div" ?) and that doesn't use the "docutils container" classes?

[choldgraf]

Yeah - though will this mean extensions that generate containers will have unforeseen behavior? I guess there's a reason that this is a problem in the first place that we should probably document it...

[stijnvanhoey]

Still work in progress, but the addition of a Div directive provides a lot of flexibility on adding bootstrap classes, e.g. following cards are setup using the div directive together with bootstrap class names:

It won't work for all use cases, e.g. using sphinx-aware links inside bootstrap buttons as on https://pandas.io/docs/ won't work out of the box with this approach.

@jorisvandenbossche would you rather abstract the entire concept of a bootstrap card in a single directive (covering an optiniated version of the cards) or are you fine in focusing on individual building blocks/divs?

[jorisvandenbossche]

Thanks for adding the div directive!

would you rather abstract the entire concept of a bootstrap card in a single directive (covering an optiniated version of the cards) or are you fine in focusing on individual building blocks/divs?

I would say that the div directive in itself is useful anyway? But in addition also having a more opinionated card directive could remove quite some boilerplate for the user of it?

[jorisvandenbossche]

We should probably also start adding tests for such things .. (in which case we should look into how sphinx is doing this, might not be the easiest thing to do ..)

[jorisvandenbossche]

Maybe can move this to a dedicated directives.py file to not have the init file too crowded?

[choldgraf]

A quick note on testing - over in some of the executable book project repos, we've starded using pytest-regressions on the doctree XML that's generated from Sphinx...this has helped simplify things quite a bit, maybe worth exploring here!

[jorisvandenbossche]

@choldgraf interesting. Could you point to an example repo that is using this?

[choldgraf]

Yep, as an example see the MyST-NB repo.

Here is the tests file, you'll see it uses pytest-regressions throughout: https://github.com/ExecutableBookProject/MyST-NB/blob/master/tests/test_parser.py#L5

Here are the XML files that are checked for regressions: https://github.com/ExecutableBookProject/MyST-NB/tree/master/tests/test_parser

Basically, you generate them once and double-check that they look correct to you, and then pytest-regressions will throw an error any time the output of running the tests causes a change to those files.

perhaps @chrisjsewell can provide advice as well?

[chrisjsewell]

I would say that the div directive in itself is useful anyway? But in addition also having a more opinionated card directive could remove quite some boilerplate for the user of it?

FYI I have essentially implemented this in: https://github.com/executablebooks/sphinx-panels

I accounted for this container issue by overriding the container visitor class:
https://github.com/executablebooks/sphinx-panels/blob/b8d6b36f946a6d86b264323189866c5e48eeff0b/sphinx_panels/__init__.py#L302
A div directive would probably be a more ideal solution though, and ideally it would be in a more standalone package than this one

[chrisjsewell]

Should "null" visitors also be set for other output formats

[jorisvandenbossche]

Sphinx-panels now added such a div directive: executablebooks/sphinx-panels#25 (in addition to also have the cards as "panel" directive as well), making this PR obsolete.

I am only wondering if we shouldn't just make sphinx-panels a dependency of this theme, so those directives are automatically available (or we should maybe at least document / showcase them in our theme docs)

[choldgraf]

+1 from me, then we can upstream bootstrappy syntax there instead of only having it in this theme

[choldgraf]

can we close this one? There's a div directive implemented in https://sphinx-design.readthedocs.io/en/latest/css_classes.html so IMO we can just recommend using it

[choldgraf]

thanks @stijnvanhoey !