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

Add initial dropdown directive implementation #15

Merged
merged 11 commits into from
May 17, 2020
Merged

Conversation

chrisjsewell
Copy link
Member

Addresses #14

It doesn't completely close it, because:

  • It doesn't add a style defaults option
  • It doesn't add a dropdown-group directive

Both of these are extra functionality that can be covered in later PRs.

@chrisjsewell
Copy link
Member Author

cc @choldgraf and @jorisvandenbossche

@choldgraf
Copy link
Member

Nice! I'll take a look hopefully this weekend. When you say dropdown-group, do you essentially mean bootstrap accordions?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented May 16, 2020

When you say dropdown-group, do you essentially mean bootstrap accordions

Yeh basically. Just a way to orchestrate the style of a group of dropdowns with a default style and adding a bottom/top margin for the group. So rather than:

.. dropdown:: My Content
    :container: mt-3 shadow
    :title: bg-primary text-white text-center font-weight-bold
    :body: bg-light text-right font-italic

    Is formatted

.. dropdown:: My Content
    :container: shadow
    :title: bg-primary text-white text-center font-weight-bold
    :body: bg-light text-right font-italic

    Is formatted

.. dropdown:: My Content
    :container: mb-3 shadow
    :title: bg-primary text-white text-center font-weight-bold
    :body: bg-light text-right font-italic

    Is formatted

You could have:

.. dropdown-group::
    :container: shadow
    :title: bg-primary text-white text-center font-weight-bold
    :body: bg-light text-right font-italic

    .. dropdown:: My Content
    
        if formatted

    .. dropdown:: My Content
    
        if formatted

    .. dropdown:: My Content
    
        if formatted

@chrisjsewell chrisjsewell linked an issue May 16, 2020 that may be closed by this pull request
@choldgraf
Copy link
Member

Took a look through the docs and the demo, I think it looks great in general. A couple quick thoughts:

  1. The "fade-in" is nice, though my subjective take is that it doesn't need the vertical movement along with the fade, the fade itself is enough IMO
  2. Did you consider using a fontawesome glyph for the chevrons?
  3. Will these containers degrade gracefully if the user wants to output to something that's not HTML? If it will cause issues, maybe this should be in the docs (and we should also document it in jupyter book)
  4. Doesn't need to be this PR, but I'd still like to think about the UI that could be used to hide/show entire blocks of content that don't have a title. Something that could be fairly minimal and non-intrusive, but still noticeable and clickable.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented May 17, 2020

Took a look through the docs and the demo, I think it looks great in general. A couple quick thoughts:

1. The "fade-in" is nice, though my subjective take is that it doesn't need the vertical movement along with the fade, the fade itself is enough IMO

Made animation configurable, see https://43-260360729-gh.circle-artifacts.com/0/html/index.html#transition-animation

2. Did you consider using a fontawesome glyph for the chevrons?

You could make the glyph configurable, but I would leave this for another PR.
I think though that this may over-complicate the matter;

  1. I assume you would have to additionally ensure the fontawesome asset was loaded on the page,
  2. Note how the SVG chevron's color will automatically match the font-color of the title (e.g. https://45-260360729-gh.circle-artifacts.com/0/html/index.html#combined-example). I don't know if you can do that with a glyph
3. Will these containers degrade gracefully if the user wants to output to something that's not HTML? If it will cause issues, maybe this should be in the docs (and we should also document it in jupyter book)

Yes. Note that the container is only "expanded" into HTML specific nodes if it is a HTML builder:

class DropdownHtmlTransform(SphinxPostTransform):
default_priority = 200
builders = ("html",)

Also note that, as with the panels, a container does not add any formatting for latex, etc.
It might be nicer to have the title+content formatted better inside a box(es), which is already a TODO in #6

4. Doesn't need to be this PR, but I'd still like to think about the UI that could be used to hide/show entire blocks of content that don't have a title. Something that could be fairly minimal and non-intrusive, but still noticeable and clickable.

See #19, and also #17 and #18 for things not covered by this PR

@chrisjsewell
Copy link
Member Author

chrisjsewell commented May 17, 2020

@choldgraf when there is no title, an ellipsis is now added, which is hidden when open. See https://48-260360729-gh.circle-artifacts.com/0/html/index.html#dropdown-usage.
This would be the "simple" fix for #19, but obviously it doesn't cover the sphinx-togglebutton use case of adding togglability to an existing container, e.g. what we do for code cells

@jorisvandenbossche
Copy link

This is really nice!

Only looked at the demo, and that is looking good.
I was just looking back to how we did the accordion in the pandas docs, and there we used collapsible cards. I will need to check if we could switch to use those dropdowns instead, but it might be that the ones in pandas are a bit too custimized.

@chrisjsewell
Copy link
Member Author

chrisjsewell commented May 17, 2020

This is really nice!

Cheers 😄

but it might be that the ones in pandas are a bit too custimized

Yeh looking at https://pandas.pydata.org/docs/getting_started/index.html#intro-to-pandas,
it would certailnly be possible (but not in this PR) to allow for configuration of the glyph and its position, i.e. the +/- on the left of the text, instead of a chevron on the right, but the button you incorporate in the titles would be a bit more tricky

@chrisjsewell chrisjsewell merged commit 3b95574 into master May 17, 2020
@choldgraf
Copy link
Member

choldgraf commented May 17, 2020

This is great - I think one path forward is to deprecate the {toggle} directive in sphinx-togglebutton, and just use that extension for turning a pre-existing block into something toggle-able (alternatively, we could just stop recommending people use the {toggle} directive directly). Then recommend people use {dropdown} for toggle-like behavior. Do you agree with that?

@chrisjsewell
Copy link
Member Author

chrisjsewell commented May 17, 2020

Well sphinx-togglebutton is your baby lol, so I'll leave it to your judgement if you want to fully deprecate or at least put links in that documentation to this one, suggesting dropdown.

There is always the issue of CSS incompatibilities with certain themes.
But this is mitigated; having now fixed #21, and implemented the nbsphinx approach to having multiple theme branches on RTD.

@chrisjsewell chrisjsewell deleted the dropdown branch September 15, 2020 05:19
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.

Add dropdown directive
3 participants