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

Collapsible namespace in sphinxext #3441

Merged
merged 6 commits into from
Oct 19, 2019

Conversation

greschd
Copy link
Member

@greschd greschd commented Oct 18, 2019

Use the HTML <details> tag to create collapsible namespaces in the sphinx extension for process documentation.

All collapsed:
image

All open:
image

We also discussed (#3300) directly making the heading clickable (without the extra "Namespace Ports" line), but I couldn't get this to work -- at least not without messing with the CSS. Although I'm not exactly an HTML wizard, so a more capable person might be more successful. Changing CSS is a bit of a risk IMO, it might break when the theme is updated, or people could decide to use a different theme for their plugin docs.

Note also: Edge doesn't support <display>, it will show like this:
image
But that should probably be resolved soon, with the Chrome-based Edge.

I wouldn't consider #3300 resolved, this is just an incremental improvement.

Dominik Gresch added 2 commits October 18, 2019 18:23
Note that this only works in Python3, because the extension used
to create the <details> tag is only compatible for Sphinx>=2.0.
So far, the description for a single port was wrapped inside both
a paragraph and a list item. This caused inconsistent vertical
spacing when namespaces and single ports were mixed. To avoid this,
the paragraph wrapping the content was removed. If the increased
spacing is desired, this should probably be done in CSS on the
list.
@greschd greschd requested a review from ltalirz October 18, 2019 20:37
@greschd
Copy link
Member Author

greschd commented Oct 18, 2019

If you're wondering about the spacing after the namespace port list, that seems to be just how that particular theme shows nested bullet lists - I did fix am inconsistency between regular ports and port namespaces, though.

ltalirz
ltalirz previously approved these changes Oct 18, 2019
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks @greschd!
Looks fine to me.

paragraph += nodes.emphasis(text=self.format_valid_types(port.valid_type))
paragraph += nodes.Text(', ')
paragraph += nodes.Text('required' if port.required else 'optional')
res = []
Copy link
Member

Choose a reason for hiding this comment

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

I guess this switch from paragraph was necessary to make the spacing consistent?
Perhaps a comment to explain this might be useful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that's exactly why. I did document that in the commit message, but since the paragraph is no longer there in the code I'm not sure a code comment would really be understandable.

Because the sphinxcontrib-details-directive requires sphinx>=2.0,
this dependency is also updated. Since that version of Sphinx is
incompatible with Python2, a version selector is added, and for
Python2 the previous version of Sphinx is kept.
The 'python' highlighting does not work in the visualising_graphs
page, because an ipython ! command (shell command) is used. Changing
the highlighting type to 'ipython' fixes this issue.
The sphinxcontrib-contentui needs updating to be compatible with Sphinx
version 2.2.0.
@greschd greschd force-pushed the details_directive_for_namespace branch from 77abc6f to 140891d Compare October 19, 2019 00:08
@greschd greschd requested a review from ltalirz October 19, 2019 00:32
Comment on lines -86 to +89
"sphinx==1.8.5",
"sphinxcontrib-contentui==0.2.2"
"sphinx==1.8.5; python_version<'3'",
"sphinx==2.2.0; python_version>='3.0'",
"sphinxcontrib-contentui==0.2.2; python_version<'3'",
"sphinxcontrib-contentui==0.2.4; python_version>='3.0'",
Copy link
Member Author

Choose a reason for hiding this comment

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

@ltalirz this is the main change compared to before: For the Python3 version, Sphinx needed to be updated (to at least 2.0) to be compatible with sphinxcontrib-details-directive, and that in turn required an update to sphinxcontrib-contentui.

@sphuber
Copy link
Contributor

sphuber commented Oct 19, 2019

Very nice @greschd , thanks for this.

We also discussed (#3300) directly making the heading clickable (without the extra "Namespace Ports" line), but I couldn't get this to work -- at least not without messing with the CSS. Although I'm not exactly an HTML wizard, so a more capable person might be more successful. Changing CSS is a bit of a risk IMO, it might break when the theme is updated, or people could decide to use a different theme for their plugin docs.

This would indeed be nice but if it comes at the risk of not working at all in some cases, I agree it is better to leave it for now.

Note also: Edge doesn't support <display>, it will show like this:

I was wondering why you would use Edge, but then I remembered ... 😉

Final question: does this work with nested namespaces? Technically your example does contain a doubly nested one, since inputs itself is a namespace, but maybe that is treated differently. Did you try putting another namespace in y for example?

@greschd
Copy link
Member Author

greschd commented Oct 19, 2019

I was wondering why you would use Edge, but then I remembered ...

I wouldn't have noticed if it wasn't mentioned in the docs for the <detail> tag 😄

Final question: does this work with nested namespaces? Technically your example does contain a doubly nested one, since inputs itself is a namespace, but maybe that is treated differently. Did you try putting another namespace in y for example?

Yeah good point -- it works, but I added a nested namespace to the test to make sure.

Screenshot from 2019-10-19 15-56-24

@ltalirz ltalirz merged commit 8456116 into aiidateam:develop Oct 19, 2019
@greschd greschd deleted the details_directive_for_namespace branch December 13, 2019 15:29
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.

3 participants