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 support for admonition text blocks #1400

Merged
merged 19 commits into from
Feb 20, 2022

Conversation

milmazz
Copy link
Member

@milmazz milmazz commented Oct 5, 2021

Hello Team, I'm sorry about the long delay around this PR. For now, this is still wip, but I still wanted to share the work done so far with you @RobertDober and @josevalim, and everyone that wants to join, of course, in case we want to discuss the Markdown syntax to support #891

I still find a bit odd that to make it work with EarmarkParser I need the following syntax:

iex(1)> text = """
...(1)> > ### title
...(1)> > {: .warning}
...(1)> >
...(1)> > bla bla
...(1)> > more bla bla
...(1)> """
"> ### title\n> {: .warning}\n>\n> bla bla\n> more bla bla\n"
iex(2)> EarmarkParser.as_ast(text)
{:ok,
 [
   {"blockquote", [],
    [
      {"h3", [{"class", "warning"}], ["title"], %{}},
      {"p", [], ["bla bla\nmore bla bla"], %{}}
    ], %{}}
 ], []}

image

Instead of this one that still looks good even when you don't use ex_doc:

image

iex(3)> text = """
...(3)> > ### title {: .warning}
...(3)> >
...(3)> > bla bla
...(3)> > more bla bla
...(3)> """
"> ### title {: .warning}\n>\n> bla bla\n> more bla bla\n"
iex(4)> EarmarkParser.as_ast(text)
{:ok,
 [
   {"blockquote", [],
    [
      {"h3", [], ["title {: .warning}"], %{}},
      {"p", [], ["bla bla\nmore bla bla"], %{}}
    ], %{}}
 ], []}

The current status is the following:

main

Light theme

image

Dark theme

image

This PR

Light theme

image

I think the main thing to do here is to improve the _ regular_ blockquote. I also believe that the header or the first child should not have that left margin. But this should be easy to fix. Suggestions are welcome :)

Dark theme

image

As you could notice, I haven't work on this theme yet; it needs lots of <3

Fixes: #891

@josevalim
Copy link
Member

This looks great! I love the direction this is going!

We plan to have a designer working on the sidebar in the next months, I can also ask him to work on the admonition blocks, but this PR helps a lot by giving us an idea of the design we want to go with. :)

assets/less/content/general.less Outdated Show resolved Hide resolved
lib/ex_doc/formatter/html/templates.ex Show resolved Hide resolved
@@ -15,7 +15,7 @@

<%= if nodes_map.tasks != [] do %>
<section class="details-list">
<h2 id="tasks" class="section-heading">Mix Tasks</h2>
Copy link
Member Author

Choose a reason for hiding this comment

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

REVIEWER: Not needed, this is injected by ExDoc.Formatter.HTML.Templates.link_heading/5

@RobertDober
Copy link
Contributor

Well my expertise is of no use here 😭, but thanks for the info, and kudos for all the work! 👏

@milmazz
Copy link
Member Author

milmazz commented Oct 5, 2021

Well my expertise is of no use here 😭, but thanks for the info, and kudos for all the work! 👏

@RobertDober I think it is, at least I have a question for you. Is it possible to set the class attribute in the same line as the header? Something like this:

image

Instead of doing the following that seems a bit odd, at least to me:

image

@RobertDober
Copy link
Contributor

RobertDober commented Oct 5, 2021

@milmazz

[...] at least I have a question for you. Is it possible to set the class attribute in the same line as the header? Something like this:

Unfortunately not I will have a look at the code if this might be easy to implement...

@milmazz
Copy link
Member Author

milmazz commented Oct 5, 2021

Updated light theme:

image

@RobertDober
Copy link
Contributor

@milmazz Here is the description what I just released as 1.4.16-pre1, tell me what you think about it.

@milmazz
Copy link
Member Author

milmazz commented Oct 5, 2021

Initial work on Dark theme:

image

@milmazz
Copy link
Member Author

milmazz commented Oct 5, 2021

@milmazz Here is the description what I just released as 1.4.16-pre1, tell me what you think about it.

That looks awesome, I will try to clone your repo and run some local tests :)

@milmazz
Copy link
Member Author

milmazz commented Oct 6, 2021

@milmazz Here is the description what I just released as 1.4.16-pre1, tell me what you think about it.

@RobertDober 🤔 I did a few tests with 1.4.16-pre1, and it seems that it is not producing the expected output, look:

iex(1)> EarmarkParser.version()
"1.4.16-pre1"
iex(2)> md = ["> ### title", "> {: .error}", ">", "> long desc"]
["> ### title", "> {: .error}", ">", "> long desc"]
iex(3)> EarmarkParser.as_ast md
{:ok,
 [
   {"blockquote", [],
    [{"h3", [], ["title"], %{}}, {"p", [], ["long desc"], %{}}], %{}}
 ], []}

First, the previous output breaks the current behavior from 1.4.9, which is the following:

iex(2)> md = ["> ### title", "> {: .error}", ">", "> long desc"]
["> ### title", "> {: .error}", ">", "> long desc"]
iex(3)> EarmarkParser.as_ast md
{:ok,
 [
   {"blockquote", [],
    [
      {"h3", [{"class", "error"}], ["title"], %{}},
      {"p", [], ["long desc"], %{}}
    ], %{}}
 ], []}

Assuming that introducing a breaking change is not an issue for you, let's try the IAL extension from the same line, which is what I'm interested in:

iex(1)> EarmarkParser.version()
"1.4.16-pre1"
iex(6)> md = ["> ### title  {:.error}", ">", "> long desc"]
["> ### title  {:.error}", ">", "> long desc"]
iex(7)> EarmarkParser.as_ast md
{:ok,
 [
   {"blockquote", [{"class", "error"}],
    [{"h3", [], ["title"], %{}}, {"p", [], ["long desc"], %{}}], %{}}
 ], []}

Here, the class error is assigned to the blockquote element, the h3 parent. Should not that class be given to the h3 block element?

Thanks for releasing a pre-candidate version btw. I appreciate it.

HTH

@milmazz milmazz marked this pull request as ready for review October 6, 2021 01:53
@RobertDober
Copy link
Contributor

RobertDober commented Oct 6, 2021

@milmazz I am not shocked by the 1.4.9 backwards incompatibility as the behavior was not according to specs as the ial was not on one line.

Then the behavior is exactly what tried to do, the IAL goes to the outermost code block and you must give it in the first line of the blockquote

I could disable IALs in the same line for blockquotes and rulers BTW if you need them for headers.

When I was implementing this I was kinda puzzeled if I should do them?

IIAC you want this

iex(6)> md = ["> ### title  {:.error}", ">", "> long desc"]
["> ### title  {:.error}", ">", "> long desc"]
iex(7)> EarmarkParser.as_ast md
{:ok,
 [
   {"blockquote", [],
    [{"h3", [{"class", "error"}], ["title"], %{}}, {"p", [], ["long desc"], %{}}], %{}}
 ], []}

But I am not clear what you want as a result for your first example?

@RobertDober
Copy link
Contributor

@milmazz or is

> ### title

your only use case?

@RobertDober
Copy link
Contributor

I did a quick change to the code which I think is what @milmazz needs.
If I am still wrong, please throw in two or three test cases here.

I also added a reference to this PR into the README
but probably there's a better way to advertise this cool new feature! I will gladly adapt the README.

Deployed as 1.4.16-pre2 and tagged on GH as v1.4.16-pre2 (so no need to clone for a quick check)

@milmazz
Copy link
Member Author

milmazz commented Oct 7, 2021

I did a quick change to the code which I think is what @milmazz needs.

@RobertDober Yes, I tried locally and that's what we needed, thank you so much <3

@milmazz milmazz requested a review from josevalim October 7, 2021 04:15
@RobertDober
Copy link
Contributor

Great to hear @milmazz
Need to straighten out some backward incompatibilites and expect to release 1.4.16 late today (UTC+2)

@RobertDober
Copy link
Contributor

@milmazz RELEASED

@milmazz
Copy link
Member Author

milmazz commented Oct 15, 2021

@josevalim @wojtekmach I just updated the PR to use the release suggested by @RobertDober. Would you mind letting me know if I should add some examples in the README file or any work remaining here?

@josevalim
Copy link
Member

@milmazz we asked for a designer to design the blocks for us, so I will let you know once they are ready. :) Then we can incorporate them and go ahead to merge it. Thanks!

@milmazz
Copy link
Member Author

milmazz commented Oct 15, 2021

@josevalim That's terrific. I'll wait then =)

@yordis
Copy link
Contributor

yordis commented Nov 28, 2021

Any updates on this one?


Btw a popular package docfx does the following https://dotnet.github.io/docfx/spec/docfx_flavored_markdown.html#note-warningtipimportant

@yordis
Copy link
Contributor

yordis commented Nov 28, 2021

GitBook uses the following:

Screen Shot 2021-11-28 at 5 02 43 AM

@josevalim
Copy link
Member

Sorry for the confusion. The design would only do the designs, someone needs to translate it to actual HTML+CSS. However, I think we can break this in two steps:

  • First, rebase this PR and change it so it uses the new remix icons. No CSS required
  • Someone will style the new text blocks

Could you take care of step 1? Then we will figure out step 2. :)

@milmazz
Copy link
Member Author

milmazz commented Feb 20, 2022

@josevalim No worries. I took care of both steps.

Screen Shot 2022-02-19 at 22 33 24

.

position: relative;
margin: 1.5625em 0;
padding: 0 1.2rem;
font-size: 1.28rem;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
font-size: 1.28rem;

I am thinking we should let the default font-size do its job so we keep everything consistent. WDYT? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking we should let the default font-size do its job so we keep everything consistent. WDYT? :)

Maybe I am wrong but does rem not just reflect a bigger font relative to the default font size, or did you mean that this style should have the default font size?

Copy link
Member

Choose a reason for hiding this comment

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

I didn’t notice the rem but yeah, what I mean is to keep it as is. :)

Copy link
Member

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

Just two minor comments and let's ship this! 💯 Also, what if you add a section in the README with examples? This way they will be available on hexdocs.pm too. It can be after auto-link before extensions.

@milmazz
Copy link
Member Author

milmazz commented Feb 20, 2022

@josevalim I just pushed more changes, it should cover your suggestions. Also, this is how the README is rendered at the moment using admonition blocks

Screen Shot 2022-02-20 at 10 11 54

Screen Shot 2022-02-20 at 10 12 05

README.md Outdated
The syntax is as follows:

```
> #### Error {\: .error}
Copy link
Member Author

Choose a reason for hiding this comment

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

@RobertDober Any advice on how to render this block as a raw piece?

iex(1)> sample = """
...(1)> ```
...(1)> > #### Error {: .error}
...(1)> >
...(1)> > text
...(1)> ```
...(1)> """
"```\n> #### Error {: .error}\n>\n> text\n```\n"
iex(2)> EarmarkParser.as_ast sample
{:ok, [{"pre", [], [{"code", [], ["> #### Error \n>\n> text"], %{}}], %{}}], []}
iex(3)>

The parser seems to "eat" the {: .error} part. I also tried with <pre>.

Copy link
Member

Choose a reason for hiding this comment

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

I tried it with <pre> and I think the issue is that earmark_parser is "eating" the new lines inside <pre>. I can take a look at it too:

<pre>
&gt; #### Error {: .error}
&gt;
&gt; This syntax will render an error block
</pre>

Copy link
Contributor

@RobertDober RobertDober Feb 20, 2022

Choose a reason for hiding this comment

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

@RobertDober Any advice on how to render this block as a raw piece?

iex(1)> sample = """
...(1)> ```
...(1)> > #### Error {: .error}
...(1)> >
...(1)> > text
...(1)> ```
...(1)> """
"```\n> #### Error {: .error}\n>\n> text\n```\n"
iex(2)> EarmarkParser.as_ast sample
{:ok, [{"pre", [], [{"code", [], ["> #### Error \n>\n> text"], %{}}], %{}}], []}
iex(3)>

The parser seems to "eat" the {: .error} part. I also tried with \<pre\>.

@milmazz

Oh that was tricky:

iex(10)> IO.puts markdown
```
> #### Error {\: .error}
>
> text

 :ok
 iex(11)> EarmarkParser.as_ast(markdown)
 {:ok,
 [
   {"pre", [], [{"code", [], ["> #### Error {\\: .error}\n>\n> text"], %{}}],
     %{}}
 ], []}

Copy link
Contributor

Choose a reason for hiding this comment

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

still not what we want, I will open an issue, but I am afraid the fix needs to wait.

Do we agree that we want IAL markdown rendered verbatim in code blocks?

If you do not need syntax highlight you can use an indented code block

iex(13)> IO.puts markdown
    > #### Error {: .error}

    >
    > text

:ok
iex(14)> EarmarkParser.as_ast(markdown)
{:ok,
 [
   {"pre", [], [{"code", [], ["> #### Error {: .error}\n\n>\n> text"], %{}}],
    %{}}
 ], []}

Copy link
Member Author

Choose a reason for hiding this comment

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

@josevalim @RobertDober pushed the workaround using an indented code block

re:

Do we agree that we want IAL markdown rendered verbatim in code blocks?

IMO the IAL markdown syntax should be raw inside a fenced code block.

Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
@milmazz milmazz merged commit 8e7ce49 into elixir-lang:main Feb 20, 2022
@milmazz milmazz deleted the feature/admonition branch February 20, 2022 18:00
@milmazz
Copy link
Member Author

milmazz commented Feb 20, 2022

@josevalim @RobertDober Finally merged. Thanks for your companionship and help during this long journey :)

@sorentwo
Copy link

Thanks for the work here everybody! This will be extremely useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Admonition text blocks support
5 participants