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

Code annotations #176

Merged
merged 19 commits into from
Oct 30, 2022
Merged

Code annotations #176

merged 19 commits into from
Oct 30, 2022

Conversation

2bndy5
Copy link
Collaborator

@2bndy5 2bndy5 commented Oct 27, 2022

resolves #170

This started working as expected on a fresh install of the branch, so I think it is ready for review.

Assert and comment about the state of last line in body.
This should detect any future incompatible changes in sphinx or pygments

remove unnecessary parent definition (dev artifact)
also add example of multiple comments using the same annotation
This follows closely to upstream behavior.
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

While I was tinkering, I added some functionality that lets the annotated list be rendered if not used (following upstream's behavior).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

I just noticed a CSS problem in upstream: the annotation button's content is off center when used within an admonition.

In our theme:
image

In upstream:
image

filed upstream: squidfunk/mkdocs-material#4558

I'm trying to say:
The annotated list markers don't matter; they just have to exist.
@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

Turns out we can't annotate rST snippets because of how the JS extracts the comments using pygments' CSS classes 💩 . The .cp class represents an rST comment which take the form of multiple <span> elements across multiple lines... I'd propose something upstream, but I'm not sure how "clean" this would be in JS.

MD comments don't seem to be supported at all in pygments (probably because they're actually embedded HTML syntax). So, they have a similar limitation upstream.

@jbms
Copy link
Owner

jbms commented Oct 28, 2022

It would be better to handle the annotations at build time using Python rather than with JavaScript, but that would require our own implantation of the feature which would be unfortunate.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

I'm reading up on the history of the feature, and I'm finding that it should be used sparingly...

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

I came across a comment from squidfunk saying he prefers JS, and the python side only gets attention when needed. Which is kinda bittersweet for us because we get features to use for free, but it limits our desired implementation sometimes.

@squidfunk
Copy link

I came across a comment from squidfunk saying he prefers JS, and the python side only gets attention when needed. Which is kinda bittersweet for us because we get features to use for free, but it limits our desired implementation sometimes.

This is a bold claim. I'm not sure where this is coming from. Yes, I prefer JS/TS over Python as a language, but I still try to solve each problem with the optimal technology. The latest flagship feature, the blog plugin, is almost entirely written in Python with little JS added. Code annotations are implemented in JS, as they need JS anyway for positioning and I'm not sure where preprocessing the HTML would be of any help. If you preprocess the markup, you will end up with a broken state if the JS fails to mount due to network or author errors. The current implementation ensures that it works when printing, works without JS and with JS. Progressive enhancement is at the heart of Material for MkDocs.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

I'd be hard pressed to find the comment now, but I'm glad to be wrong about that.

It's true you need JS for the tooltip positioning, but we could parse the literal block in python with pygments to find the annotations and replace them (or the entire comment) with the buttons that tie into the JS - docutils is very flexible in this regard, not sure about mkdocs though. This approach may collide with the clipboard.js functionality as well.

TBH, I'm just glad to be able to offer this feature at all here. I had a lot of fun playing with it to create the new doc. I used a "have fun and break things" approach, so users can actually see what a mistake looks like before they skip reading all the paragraphs and break it themselves.

@squidfunk
Copy link

I'd be interested to see how you pull this off preprocessing it during build time, keeping non-JS compatibility by following the principle of progressive enhancement. Feel free to create a PR upstream, but please note that I can only merge it if it actually improves the situation without losing any of the functionality we currently have. From my experience, I don't believe there's much room for improvement, but I'd be very happy to be proven wrong.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

It would be dependent on using docutils' rST parser, so I'm not sure if it could be extended to mkdocs... I intend to start reading up on pymdownx to find better ways to contribute upstream (really excited about that new block syntax), but my passion strongly sides with Sphinx because it handles API parsing out of the box.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

@squidfunk FYI, Doxygen does something similar for annotating the src code (in their "source code browser" feature). All the tooltips there are hidden divs at the bottom of the code block (not generated by JS), and they just use JS (actually JQuery) to move the div/tooltip into view (and un-hide it). This is how I'm envisioning the build-time approach.

@squidfunk
Copy link

How does that work without JS and when printing? When all tooltips are at the bottom, all context is lost when printing or when viewing without JS. For example, see this print of our getting started guide, specifically "previewing as you write".

Creating your site - Material for MkDocs.pdf

Context is preserved, because it's always a code block + a list. Anyway, if you can pull something off that preserves all of the behavior we have with preprocessing the markup, which has actual benefits to our current approach, I'd be interested.

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

At the risk of sounding like a noob, I have to ask what you mean by "print" or "printing"?

Much of Doxygen's site generation is very poorly designed. Their v2 is supposed to allow use of template engines like django, but I have yet to see that released.

Doxygen's annotations aren't customizable (for the most part), so they just use CSS to 1animate the tooltip visibility and 2fix most of the tooltip's contents relative to the div's position. Then its just a matter of where the mouse is within the viewport; they don't have a problem with the "bottom of the page scenario" (squidfunk/mkdocs-material#3639).

As for the PDF writer, I think it might be easier to leave the (1) in the literal block and render the tooltip contents as usual (like what I see in that link). We haven't been focusing on Sphinx' Latex writer, so I'd be surprised if everything works for this port of the theme (the CI doesn't complain when it runs the Latex writer though).

@2bndy5
Copy link
Collaborator Author

2bndy5 commented Oct 28, 2022

If you're curious to see the Doxygen approach rendered, I still use it for the RF24 libs.

@jbms
Copy link
Owner

jbms commented Oct 30, 2022

Thanks. Looks good to me.

@2bndy5 2bndy5 merged commit 00407a8 into main Oct 30, 2022
@2bndy5 2bndy5 deleted the code-annotations branch October 30, 2022 05:18
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.

content.code.annotate feature not working
3 participants