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 custom heading adapter #266

Merged
merged 11 commits into from
Jan 9, 2023
Merged

Conversation

lucperkins
Copy link
Contributor

@lucperkins lucperkins commented Jan 7, 2023

Fixes #267.

This PR adds a custom heading adapter akin to SyntaxHighlighterAdapter that can be used as the basis for plugins. Let me know if this is something you'd like to add and if you have any feedback on the API. To give an example, I have a project where I'd like to add some Tailwind/AlpineJS goodies directly into my headings, like this:

<h2 id="some-heading" x-data="{ showAnchor: false }" @mouseover="showAnchor = true" @mouseout="showAnchor = false">
  Some heading
  <a href="#some-heading" class="ml-2" x-show="showAnchor">#</a>
</h2>

One of many possible use cases, of course, but always nice to have a lever to pull 😄

Come to think of it, enabling custom headings would be a way to avoid issues like #250 and #261 without hard-coding API changes.

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

Outstanding. It occurs to me we probably want to let the heading adapter also take care of rendering the close tag? Otherwise they're restricted to only leaving open the exact <h1><h6> corresponding to the level given, since that's what we then close on the other side. This rules out uses where users may wish to transform the levels (e.g. render # as an <h4>), use a different tag entirely, nest the heading content in multiple tags, etc.

src/tests.rs Outdated Show resolved Hide resolved
src/html.rs Outdated Show resolved Hide resolved
src/html.rs Outdated Show resolved Hide resolved
@kivikakk
Copy link
Owner

kivikakk commented Jan 8, 2023

Let me know what you think of my suggestion; happy to merge whenever you're happy with what you've got, just mark as ready.

@lucperkins
Copy link
Contributor Author

lucperkins commented Jan 8, 2023

@kivikakk So, I chose not to let the adapter render the closing tag because I'm not quite sure how to pass in the AST content of the heading to the adapter. Take this heading as an example:

## Here is some `code` and some **bold** and some *italics* and even ~strikethrough~

In this case, you wouldn't want just the string "Here is some code and some bold and..." You'd want those nodes, too. If there's a good way to provide that, then we could indeed provide full control over the rendering. In my adapter implementation I pass over content as a string basically so that people can provide anchors but I leave the Markdown content untouched.

Update: I've come up with something that might work. As you can see, the adapter trait now has an enter method and an exit method. This preserves the content of the heading while providing total control over everything that wraps that content. I think it still might be better to just provide total control and pass in the level and the AST content and expect a string back. But I'm not quite sure how to do that.

@lucperkins lucperkins marked this pull request as ready for review January 8, 2023 15:22
@lucperkins lucperkins marked this pull request as draft January 8, 2023 16:08
@kivikakk
Copy link
Owner

kivikakk commented Jan 9, 2023

As you can see, the adapter trait now has an enter method and an exit method. This preserves the content of the heading while providing total control over everything that wraps that content. I think it still might be better to just provide total control and pass in the level and the AST content and expect a string back. But I'm not quite sure how to do that.

I think this is perfect for now — giving the adapter the entire sub-AST to render could be problematic, because presumably it'd need some way to call back into the renderer (with appropriate state) if it just wanted to do business as usual. If a need for more complicated transforms comes up in the future, we can deal with it then.

@kivikakk
Copy link
Owner

kivikakk commented Jan 9, 2023

I'm happy to merge this if you are. 👍

@lucperkins lucperkins marked this pull request as ready for review January 9, 2023 12:20
@lucperkins
Copy link
Contributor Author

lucperkins commented Jan 9, 2023

@kivikakk Happy to merge as well 😄 I did add some tests and bulked up the docs a teensy bit in the meantime if you want to take a look.

Copy link
Owner

@kivikakk kivikakk left a comment

Choose a reason for hiding this comment

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

Awesome. Thank you so much!! 😊❤️

@kivikakk kivikakk merged commit 2e219a9 into kivikakk:main Jan 9, 2023
@lucperkins lucperkins deleted the heading-adapter branch August 15, 2023 02:46
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.

Provide custom heading adapter
2 participants