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 topic directive #1132

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Apr 18, 2024

This PR adds the RST topic directive, which behaves like an aside, with a stylised heading.

Here's an example RST:

.. topic:: Bees Talk About Work Life Balance
  :class: 

   Bzzbzz bzz bzzbzz bzz bzzbzzbzzzzz!!!?? Bzz.

This generates the following HTML in Sphinx/docutils:

<aside class="topic some-class">
  <p class="topic-title">About Bees</p>
  <p>Hello bees!</p>
</aside>

Our MDAST with this PR is

{
  "type": "aside",
  "kind": "topic",
  "children": [
    {
      "type": "paragraph",
      "children": [
        {
          "type": "text",
          "value": "Bees Talk About Work Life Balance"
        }
      ]
    },
    {
      "type": "paragraph",
      "children": [
        {
          "type": "text",
          "value": "Bzzbzz bzz bzzbzz bzz bzzbzzbzzzzz!!!?? Bzz."
        }
      ]
    }
  ],
  "class": "some-class"
}

I welcome any suggested changes here.

One extraneous change is the addition class and label (identifier) attributes to the aside AST node type. I think it's our goal to add this to all MyST nodes, right? Perhaps that would be done on the base Node type, rather than per-node like we currently do in myst-spec?

@agoose77 agoose77 force-pushed the agoose77/feat-add-topic-directive branch from 9a218f2 to 1bc4109 Compare April 18, 2024 13:24
@agoose77 agoose77 requested a review from rowanc1 April 18, 2024 13:38
@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 19, 2024

In sphinx this looks like:

image

I think we should handle the header properly. I think that this should eventually be title as a node type, and be shared between admonitions, drop-downs, topics, exercises, etc. But for this PR we should adopt admonitionTitle and then follow up with fixing #1139.

As another bug, we are dropping other aside titles.

@rowanc1 rowanc1 force-pushed the agoose77/feat-add-topic-directive branch from d7e1e55 to eaf2d9a Compare April 19, 2024 20:33
@agoose77
Copy link
Contributor Author

This PR avoids the new node type by assuming that the first child is the title. I don't hate that, but I recognise that we long-term want #1139

@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 19, 2024

Looking through this, I think we just want to merge with aside and expand to supporting optional titles in the others. Right now that is a bug, and then expanding that, both are the same anyways.

https://jupyterbook.org/en/stable/content/layout.html#sidebars-within-content

@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 19, 2024

Making it look like this:

image

I think there are some improvements to be made, but I think this is probably good enough for a first cut?

rowanc1 added a commit to jupyter-book/myst-theme that referenced this pull request Apr 19, 2024
@rowanc1 rowanc1 changed the title πŸ’… πŸ–ΌοΈ Add topic directive πŸ’… πŸ–ΌοΈ Add topic directive Apr 19, 2024
@rowanc1
Copy link
Collaborator

rowanc1 commented Apr 19, 2024

Going to move ahead with this for now to queue up a release - we can always come back and make improvements.

@rowanc1 rowanc1 merged commit f656e57 into main Apr 19, 2024
5 checks passed
@rowanc1 rowanc1 deleted the agoose77/feat-add-topic-directive branch April 19, 2024 21:23
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.

2 participants