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

NEW: Add sidenote and marginnote syntax #546

Merged
merged 48 commits into from
May 19, 2022

Conversation

AakashGfude
Copy link
Member

@AakashGfude AakashGfude commented Mar 30, 2022

Taking @mmcky and @choldgraf's comments into account. Implemented the sidenote and marginnote using footnote syntax as the first step. Changing footnote nodes to sidenote and inline element nodes in a post-transform. This does not create any new syntax to write sidenotes and uses the already available footnote syntax. To convert all footnotes to sidenotes, the config variable is - use_sidenotes: True under html_theme_options. To convert a sidenote to marginnote (i.e. not haveing superscripted numbers), use {>} at the start of the footnote source text (similar syntax was first proposed in #523 (comment)).

Example:

Source Code:

On wide screens, margin content will pop out to the side of the page and allow content underneath it to move upwards[^myref].
This allows you to provide extra information without interrupting the flow of information.

There are many other things that you can do with the Sphinx Book Theme.
Now that you've gotten a start, check out the other sections to the left to learn more about how to use it[^3].

[^myref]: Some footnote text

[^3]: And here's a longer margin block-level element...
      :::{note}
      ...with a note inside.
      :::

Footnote:

Screen Shot 2022-04-06 at 4 55 48 pm

Sidenote:

Screen Shot 2022-04-06 at 4 55 18 pm

When modifying the above source code with:

[^myref]:  {>} Some footnote text

The output is:

Screen Shot 2022-04-07 at 10 53 04 am

Note that sidenotes do not support any directives inside it as it is an inline element. Tufte also uses inline span elements. The reason span makes more sense than aside in my opinion is mentioned in the 31st March description below.

NOTE: The functionality of clickable text/icon on smaller screens as was implemented in #523, is also present in this PR.

31st march

Was trying to implement sidenote leveraging the footnote code and syntax as discussed in #523 . I tried going through the docutils code, where the footnote is implemented.
I have a few concerns which I wanted to discuss:

  • the insertion of footnote node in the document happens during the parsing phase I think. And possibly the only place to rearrange the document to make the footnote node move near the footnote-reference is during a post-transform.

  • It was much easier to deal with span elements we generated here A simple sidenote, marginnote functionality #523 for sidenotes, and which Tufte CSS uses as well, to position the sidenote. As span can go inside a p tag, and other similar tags, which will have the sidenote reference inside it. And so relative positioning is pretty straightforward. The aside tag on the other hand, when trying to insert inside a 'p' tag in post-transform, does not work, and it is always inserted after the p tag, which makes it hard to position.

cc: @choldgraf @chrisjsewell @mmcky @rowanc1

@AakashGfude AakashGfude marked this pull request as draft March 30, 2022 13:23
@AakashGfude AakashGfude marked this pull request as draft March 30, 2022 13:23
@mmcky
Copy link
Member

mmcky commented Apr 1, 2022

@AakashGfude I have been taking a look at this PR in addition to #523 and from a developer perspective it seems to me that it would make sense to:

  1. Develop the concept of a sidenote as a feature with supporting visit and depart methods for html and latex
  2. Add a global option footnote_as_sidenote: [True/False] which enables a sphinx.transform to move footnote nodes (visually) to sidenote nodes in the AST to be parsed by sidenote writers.

To me this flow seems more logical rater than overloading the footnote objects to create sidenote style of output. Some of the benefits of this approach are improvements made to sidenote feature are automatically gained if we use a transform to move footnotes to sidenote nodes in the AST.

@AakashGfude
Copy link
Member Author

Thanks, @mmcky, that sounds like a reasonable way to go. I will approach it that way and see how it feels.

@AakashGfude AakashGfude changed the title Sidenote using footnote code of docutils Sidenote using footnote syntax Apr 5, 2022
@AakashGfude
Copy link
Member Author

made changes, and updated the top description.

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This is looking nice! Much cleaner than having lots of new roles/directives, I think. A few quick thoughts:

  • I pushed a commit that added the extra docs from your other PR, and updated them a bit for this new syntax.
  • I updated the "marginnote" prefix to {-} instead of {>} to be in-line with what the pandoc-sidenotes extension uses.
  • I've added some example usage in our reference docs as well so we can show off the behavior there.
  • I noticed that having multiple marginnotes in the same sentence does not work on mobile screens, I think it's because the for is pointing to the same thing in both cases, can you fix that?
  • I think that it'd be good to have some visual cue that the numbers/symbols are "clickable" on mobile - maybe they can still be blue? Or some other way to suggest they can be clicked?

@AakashGfude
Copy link
Member Author

AakashGfude commented Apr 7, 2022

Thank you so much @choldgraf, for the additions, very helpful.

* I noticed that having multiple margin notes in the same sentence does not work on mobile screens, I think it's because the `for` is pointing to the same thing in both cases, can you fix that?

Have pushed the fix now.

* I think that it'd be good to have some visual cue that the numbers/symbols are "clickable" on mobile - maybe they can still be blue? Or some other way to suggest they can be clicked?

Yes, I think the original color (blue) seems like a good option. As the user is already used to assuming that it is a clickable reference? Have pushed the change. Let's see how it feels.

@choldgraf
Copy link
Member

choldgraf commented Apr 8, 2022

I think the current implementation is pretty good to me. The behavior is nearly identical to how pandoc-sidenotes works, so we're not reinventing the wheel from a UX perspective or anything.

One thing I noticed: If you have a marginnote in between two sidenotes, it still takes up a "number" even though it is not actually numbered (so the sidenotes will jump from 1 to 3 even though there is no 2). Is that easy to fix? If not it's not a blocker on this PR in my opinion.

Actually while we're at it, we might as well note in the docs that the UX of this is inspired by pandoc-sidenote (https://github.com/jez/pandoc-sidenote)

src/sphinx_book_theme/nodes.py Outdated Show resolved Hide resolved
@AakashGfude
Copy link
Member Author

One thing I noticed: If you have a marginnote in between two sidenotes, it still takes up a "number" even though it is not actually numbered (so the sidenotes will jump from 1 to 3 even though there is no 2). Is that easy to fix? If not it's not a blocker on this PR in my opinion.

ooh yeah, it's because we are converting footnotes to sidenotes/margin notes and using the numbering of footnotes. margin notes were also footnotes and had a number.

src/sphinx_book_theme/_transforms.py Outdated Show resolved Hide resolved
@AakashGfude
Copy link
Member Author

@choldgraf I think you will be the best person to resolve the conflict, as you had written the doc file. :)

@choldgraf
Copy link
Member

OK I think I fixed up the docs conflict, wanna take a look and let me know if it reads OK to you?

Other than that, anything else you'd like to tackle here?

@AakashGfude
Copy link
Member Author

@choldgraf the doc looks good to me. Thank you!
I could not use the d-none class instead of d-n like you mentioned here #546 (comment). As d-none class in pydata-sphinx-theme has display:none !important in it, which is preventing it from being overridden.

Else I think rest is good.

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

OK this looks good to me - let's merge it in and see how folks like using it, and we can iterate from there!

@choldgraf choldgraf merged commit af6277a into executablebooks:master May 19, 2022
@choldgraf
Copy link
Member

Thanks very much @AakashGfude for this addition!

@choldgraf choldgraf added the enhancement New feature or request label May 19, 2022
@choldgraf choldgraf changed the title Sidenote using footnote syntax NEW: Sidenote using footnote syntax May 19, 2022
@choldgraf choldgraf changed the title NEW: Sidenote using footnote syntax NEW: Add sidenote and marginnote syntax May 19, 2022
@AakashGfude
Copy link
Member Author

Thank you, @choldgraf.

@AakashGfude AakashGfude deleted the footnote-sidenote branch May 20, 2022 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

4 participants