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

[WIP] allow toggling inputs and outputs #436

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

machow
Copy link

@machow machow commented Apr 15, 2020

This (WIP) PR addresses #303.

I took a quick look over nbsphinx, and wanted to highlight some challenges and decisions involved. (I'll clean up / flesh out the doc notebook before wrapping up).

Changes

  • NbOutput directive accepts :class: option, puts it on outer classes
  • template checks for "hide_input" tag, if found adds the "toggle" class.
  • sphinx-toggle added to extensions

Example

Hidden:

image

Toggled:

image

Questions

  • the toggle class (or another class name nbsphinx might want) has to be set on the outer container. Right now I use the :class: option, but NbInput uses that option for the inner container. Is it worth adding a toggle option to the NbOutput and NbInput directives?
  • does using the metadata.tags "toggle_input" and "toggle_output" sound good?
  • other things you'd want from the PR / things to look out for?

(As a general note, a generic metadata option to specify classes to set on either output or input containers would also allow toggling.)

@mgeier
Copy link
Member

mgeier commented Apr 16, 2020

Thanks for this PR!

What I really like about it, is that the inclusion of the sphinx_togglebutton extension is optional.
This way it is open for users to create their own implementation of the HTML/CSS/JavaScript details.

I think the same flexibility should be used for selecting the relevant notebook tags.

I think there are 3 categories of tags:

  • affecting code inputs
  • affecting code outputs
  • affecting whole cells (including code cells, but also others)

On top of those 3 categories, there could be an option to choose whether the toggleable parts are initially visible or not.

Or do you think they should always be initially hidden?

Or should they be initially hidden by default, but with an additional tag this can be changed to initially visible?

I think it would be great to make those 4 (or 3) sets of tags configurable (each accepting either a single string or multiple strings).

Those options should of course have reasonable defaults.
I think we should look around and check what tag names are used in other projects.
If we use sets of tags, we could even support multiple default tags for each category.

the toggle class (or another class name nbsphinx might want)

I think it would make sense to keep the CSS class name used by sphinx_togglebutton.
Otherwise it wouldn't work out-of-the box, right?

We could probably make the CSS class(es) configurable as well, but I think this might be overkill?

has to be set on the outer container. Right now I use the :class: option, but NbInput uses that option for the inner container. Is it worth adding a toggle option to the NbOutput and NbInput directives?

It would sure be a possibility. Feel free to add options there.

However, I can think of another way to implement this:

You could insert the CSS classes directly into the RST source like this:

{%- if ... whatever ... %}
.. rst-class:: toggle
{%- endif %}

{%- if ... some condition ... %}
.. rst-class:: toggle-hidden
{%- endif %}

.. nboutput::
    ...

Those rst-class classes are automatically applied to the following HTML element.
This way you wouldn't need any change in the implementation of NbInput and NbOutput.

I'm not sure which approach would be simpler in the end.

As a general note, a generic metadata option to specify classes to set on either output or input containers would also allow toggling.

I don't understand how this is supposed to work, can you please elaborate?

Finally, there is one thing I'm wondering about:
If there are multiple outputs in one code cell, should they be individually toggleable or all at once?
I guess former would be easier to implement. For latter we would have to wrap all outputs into an additional container, right?

@machow
Copy link
Author

machow commented Apr 18, 2020

Thanks for your quick feedback. I'll take a closer look Monday--but wanted to quickly follow-up :).

What I really like about it, is that the inclusion of the sphinx_togglebutton extension is optional. This way it is open for users to create their own implementation of the HTML/CSS/JavaScript details.

This really stood out to me while looking through possible implementations, too. I think the most remarkable thing is that it's basically a question of how to put a CSS class in the right spot. If users could add CSS classes to parts of a code cell (eg input, output, whole cell), nbsphinx could push this feature to "userland"!

For example, if users can add CSS classes, nbsphinx gets this feature below for free...

Or should they be initially hidden by default, but with an additional tag this can be changed to initially visible?

Since sphinx-togglebutton has a class to show the toggled section by default!

image

In a sense then, the ability for users to add CSS classes in 3 code cell spots would make up an interface for extensions, like sphinx-togglebutton, which tells...

  • extension writers: if your extension is configured via CSS classes, it works with nbsphinx.
  • users: you can use any extensions that work as above, and use their documentation.
  • many feature requesters: make or modify an extension.

This is what I was stewing on with the comment below :).

As a general note, a generic metadata option to specify classes to set on either output or input containers would also allow toggling.

I don't understand how this is supposed to work, can you please elaborate?

But I realize nbsphinx has the broader jupyter notebook metadata ecosystem to be mindful of. I'm definitely on board with helping out on an implementation that uses a more general interface, or is sphinx-togglebutton specific.

As a concrete example, suppose that nbphinx added tags (or some other metadata field) as classes on both the .nbinput and .nboutput containers (not saying this is the best idea!), and set this in conf.py.

# sphinx-togglebutton option
togglebutton_selector = ".nbinput.toggle_input, .nb_output.toggle_output"

Then users adding the tags toggle_input and toggle_output would enable toggling cells.

@machow
Copy link
Author

machow commented Apr 18, 2020

cc @choldgraf, since he mentioned

All you should need to do is modify the selectors for the toggle buttons so that they map on to nbsphinx HTML structure, as described here: https://sphinx-togglebutton.readthedocs.io/en/latest/#control-the-selector-text-used-to-make-elements-toggle-able

And I'm wondering now if he was thinking about how it would look if nbsphinx added CSS classes to parts of the cell, based on something in metadata (eg how jupyterbook adds tag_<tag_name>; see description above). Curious how well this has worked for jupyterbook!

@choldgraf
Copy link

@machow quick thoughts from Jupyter Book - I think the tags work nicely, but it's still very early days there. Jupyter doesn't have a strong "tagging" culture yet, I think because only until recently the UIs didn't make it easy to tag things. In Jupyter Book, we are recommending that people use Directives for controlling things like "toggle this content" with the book content, and they use tags to trigger the same behavior just for code cells (since that's the only way I know of to attach metadata/triggers to a code cell)

@mgeier
Copy link
Member

mgeier commented Apr 21, 2020

@machow

suppose that nbphinx added tags (or some other metadata field) as classes on both the .nbinput and .nboutput containers (not saying this is the best idea!)

I'm not sure if I understand.
Are you saying that nbsphinx would in this scenario take all cell tags and unconditionally emit them as (potentially tag_-prefixed) CSS classes?

I think it would make more sense to have configurable lists of tags that nbsphinx would turn into some pre-defined CSS classes. But I'm open for other suggestions!

@sandipde
Copy link

Following. I think this would be super useful. Please have an option to add custom tags list too.

@mgeier mgeier mentioned this pull request Sep 23, 2020
@mgeier
Copy link
Member

mgeier commented Oct 20, 2020

@machow Any news on this?

@mgeier
Copy link
Member

mgeier commented Mar 15, 2021

@machow Do you want to continue working on this?

In the meantime there has been a PR (#543) that removes cells instead of hiding/toggling.

It would be great if in the end both things would work, hopefully without interfering with each other ...

@nvaytet
Copy link

nvaytet commented Aug 15, 2021

I would also really welcome this addition to nbsphinx. Thanks in advance

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.

None yet

5 participants