Skip to content

Adjust docutils container class addition #92

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

Adjust docutils container class addition #92

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
500af67
Adjust docutils container class addition
stijnvanhoey Feb 17, 2020
5b41305
Merge branch 'master' of github.com:pandas-dev/pydata-bootstrap-sphin…
stijnvanhoey Mar 9, 2020
03c16e4
Remove docutils container removal hack
stijnvanhoey Mar 9, 2020
5b234b1
Add Div directive
stijnvanhoey Mar 9, 2020
395034c
Add div bootstrap usage example
stijnvanhoey Mar 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pandas_sphinx_theme/bootstrap_html_translator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,3 +62,10 @@ def visit_table(self, node):
# classes.append('align-%s' % node['align'])
tag = self.starttag(node, "table", CLASS=" ".join(classes))
self.body.append(tag)

def visit_container(self, node):
"""
Remove 'docutils' and 'container' classes to overcome interference
between docutils container class and bootstrap container class
"""
self.body.append(self.starttag(node, "div", CLASS=""))
Copy link
Collaborator

@choldgraf choldgraf Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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))

Copy link
Member

@jorisvandenbossche jorisvandenbossche Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.