tommilligan / mdbook-admonish

A preprocessor for mdbook to add Material Design admonishments.
MIT License
171 stars 18 forks source link

Smart quotes doesn't work inside admonish blocks #1

Closed schungx closed 2 years ago

schungx commented 2 years ago

1) Standard quotes are not turned into smart quotes inside the blocks even with the mdbook option turned on.

2) Also, it would be nice to be able to style the title text... in other words, specify the title text also as MarkDown...

3) Also, inlined HTML tags are not recognized:

image

4) Code blocks inside admonish blocks must be wrapped with ~~~ otherwise they conflict with the admonish closing tag

5) Also, symbols inside fenced code blocks inside admonish blocks are not recognized:

image

tommilligan commented 2 years ago

Thanks for checking out the project - this is a very helpful QA for cases I haven't tried yet!

To reply to your points in order:

1) Standard quotes are not turned into smart quotes inside the blocks even with the mdbook option turned on.

Could you provide an example snippet and your expected result for this? I haven't used smart quotes in commonmark before, and don't know which mdbook option you are referring to.

Also, point 3 may be related if you're referring to ".

2) Also, it would be nice to be able to style the title text... in other words, specify the title text also as MarkDown...

Agreed. When I added support for body markdown last night, I tried adding it for the title too and I broke the styling as a result. So I think this is doable, just requires some poking with the CSS probably.

3) Also, inlined HTML tags are not recognized:

image

The plugin code I based this on deliberately escaped the HTML characters <>*". This is certainly one reason why it isn't recognising them, so I'll start there and investigate onwards (I don't know if there are any limitions on the nesting depth of md > html > md > html)

4) Code blocks inside admonish blocks must be wrapped with ~~~ otherwise they conflict with the admonish closing tag

I think this is expected behaviour. The plugin uses the markdown AST, so if you prefer you can invert the syntax and make the adminsh block use tildes instead:

~~~admonish info

This should work?

~~~

5) Also, symbols inside fenced code blocks inside admonish blocks are not recognized:

image

See point 3.


Again, thanks very much for the feedback! I'll investigate further in the next few days, or PRs are welcome if you'd like.

schungx commented 2 years ago

Smart quotes:

[output.html]
curly-quotes = true

I think the main issue is that the text within the block is fed to you already escaped (e.g. < turns to &lt;). If you feed the same thing to a MarkDown processor again, it gets the current result.

I suggest that you "un-escape" the text you get within the block to get back to the original MarkDown text.

tommilligan commented 2 years ago

Pushed fixes for 2, 3 and 5. Added an example for 4 in the README. This is all now released as v1.3.0.

I couldn't reproduce or find the issue with smart quotes, but if you let me know further details I'm happy to take a look.

schungx commented 2 years ago

See point 3.

Point no.5 is not related to point no.3. No.3 is about things like &ndash; (which I have in the MarkDown text) not being processed.

No.5 is about normal text being turned into escaped.

schungx commented 2 years ago

This is all now released as v1.3.0.

That was fast. Will try it out and report back!

schungx commented 2 years ago

I think you made a typo somewhere:

image

The source for this looks like this:

```admonish warning "Features are not additive"

Most Rhai features are not strictly _additive_, i.e. they do not only add optional functionalities.
```

Looks like the text is incorrectly wrapped in <pre><code>...</code></pre>.

schungx commented 2 years ago

It certainly looks like the FIRST line of each block (title or content) is wrapped as a code block.

But only the first line.

<div class="[admonition info]()">
  <div class="[admonition-title]()">
    <p>
<pre><code>NativeCallContext parameter

&lt;/p&gt;
</code></pre>
</div>
  <div>
    <p>
<pre><code>The _first_ parameter of a function can also be [`NativeCallContext`], which is treated specially by
</code></pre>
<p>the plugins system.</p>
<pre><code>&lt;/p&gt;
</code></pre>
</div>
</div>

comes from this:

```admonish info "NativeCallContext parameter"

The _first_ parameter of a function can also be [`NativeCallContext`], which is treated specially by
the plugins system.
```
tommilligan commented 2 years ago

Well, that took some random intuition to pin down. It turned out indenting the generated HTML by 4 spaces causes it to be picked up the the markdown code block rendering - previously it was only indented by 2 spaces at most, so it didn't get caught by it. Fun!

When I have some more time on Monday, I might look into adding an integration test that looks at the final generated book after subsequent mdbook passes, to avoid issues like this. Currently it's just the preprocessing step I have unit tests for.

Thanks for the info on how to reproduce the smart quotes - with v1.3.1 I think this is fixed:

```admonish info "Title"
A beautifully "styled" message &ndash; with ndash.


![image](https://user-images.githubusercontent.com/12255914/154795882-9e49692a-87bd-422c-a4bf-94d48a767d21.png)
schungx commented 2 years ago

Wonderful! Looking good! Will try to test some more combinations!

schungx commented 2 years ago

One thing you'd need to put into your docs:

Since the title is parsed as a JSON string, and you have to take MarkDown escapes into account, in order to put a quote character " into the title, you'd need two backslashes: "This is a title with \\"two\\" quotes."

Both \\ turns into a single \ by MarkDown, then \" turns into a quote by JSON.

Another one:

To put a back-tick ` into a title, annotate the block with ~~~ instead of ```

schungx commented 2 years ago

Titles terminating with non-ASCII letters seem to have trouble:

```admonish danger "Don't Do It™"

image

Remove and it works fine. Replacing with &trade; works fine.

Adding more text after is the same.

I think it is failing to interpret the UTF-8 streams for non-ASCII letters. Replacing with other non-ASCII characters yield similar problems.

tommilligan commented 2 years ago

Thanks again! Both fixed in the latest release 1.3.2.

The title issue was due to a byte/char index mismatch that was causing an incorrect slice. This could also potentially cause a panic if the slice happened to be mid-char boundary, so good catch!

I opened #3 to track adding property based tests to catch these kinds of cases, which I'd like to do in future.

Thanks for all your help QAing this and getting the rough edges sanded off, I really appreciate it. I'll close this issue now as I think all the original bugs are squashed, but please do open more if you find them!

schungx commented 2 years ago

This tool is looking great!

I always dislike the bland, boring look of mdbook and this one works wonders!

You can now find a practical example using your work here: https://rhai.rs/book/