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

Last item in lists is not wrapped in <p> #93

Closed
ericmj opened this issue Sep 29, 2016 · 14 comments
Closed

Last item in lists is not wrapped in <p> #93

ericmj opened this issue Sep 29, 2016 · 14 comments
Assignees

Comments

@ericmj
Copy link

ericmj commented Sep 29, 2016

You can see the formatting issue on https://hex.pm/docs/tasks#hex_publish.

@RobertDober
Copy link
Collaborator

Frankly I cannot. Can you provide markdown input and expected html output?

@ericmj
Copy link
Author

ericmj commented Sep 29, 2016

You can see that some of the text in lists is bold and some is not. If you cannot see it on the website you can inspect the source.

Here is the markdown: https://github.com/hexpm/hex/blob/master/lib/mix/tasks/hex/publish.ex#L39-L52

@RobertDober
Copy link
Collaborator

RobertDober commented Sep 29, 2016

I succeeded to extract an offending case

iex(30)> md = """
...(30)> # HL
...(30)>
...(30)>   * one
...(30)>
...(30)>   * two
...(30)> """
 "# HL\n\n  * one\n \n  * two\n"
iex(31)> Earmark.to_html md
"<h1>HL</h1>\n<ul>\n<li><p>one</p>\n</li>\n<li>two\n</li>\n</ul>\n"

@RobertDober RobertDober self-assigned this Sep 29, 2016
@RobertDober RobertDober added this to the 1.0.2 milestone Sep 29, 2016
@ericmj
Copy link
Author

ericmj commented Sep 29, 2016

I was meaning to find a reproducing case but got bogged down on an unrelated issue. I am sorry for making you find it.

Thank you @RobertDober ❤️

@RobertDober
Copy link
Collaborator

np @ericmj I just needed to read the title...

@RobertDober
Copy link
Collaborator

RobertDober commented Oct 2, 2016

I kinda have a fix for this, honestly the semantics of list items in paras goes a little over my head right now.
Neither from the code, nor from experiments on Babelmark I managed to really understand when to put them and when not.

Therefore, for starters I decided all <li> are wrapped in <p>s if one is, somehow defying the parser's semantics in the renderer. Ugly and maybe incorrect.

@pragdave your guidance would be appreciated.

And anybody I would be grateful for some pointers or explanations on that account.

@pragdave
Copy link
Owner

pragdave commented Oct 3, 2016

My understanding is that the

s are added if there is a blank line after
the item.

On Sun, Oct 2, 2016 at 5:43 AM, Robert Dober notifications@github.com
wrote:

I kinda have a fix for this, honestly the semantics of list items in paras
goes a little over my head right now.
Neither from the code, nor from experiments on Babelmark I managed to
really understand when to put them and when not.

Therefore, for starters I decided all

  • are wrapped in

s if one is, somehow defying the parser's semantics in the renderer. Ugly
and maybe incorrect.

@pragdave https://github.com/pragdave your guidance would be
appreciated.

And anybody I would be grateful for some pointers or explanations on that
account.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#93 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAApmKWWucOJ1zV4v8Jqi7k0BmTa6AGZks5qv4q1gaJpZM4KKbKJ
.

@RobertDober
Copy link
Collaborator

RobertDober commented Oct 3, 2016

Thank you @pragdave I will review the code a little more to be sure to not have violated that principle. After merging this issue branch it might be a good idea to release 1.0.2.

@ericmj
Copy link
Author

ericmj commented Oct 3, 2016

My understanding is that the <p>s are added if there is a blank line after the item.

Don't you always have to terminate the list with a blank line after the last item?

@RobertDober
Copy link
Collaborator

@ericmj that is true, and BTW, if you are interested in technical details, it was the source of the bug, as the parser did not put the "spaced" flag on the last list item, never. And it is this never which still bugs me.

I guess one can trust the tests and the code is releasable in case the bugfix is urgently needed. Otherwise I'd like to investigate some further, but that will take some time.

@ericmj
Copy link
Author

ericmj commented Oct 3, 2016

A bugfix is not needed urgently so on my part please take your time.

@RobertDober
Copy link
Collaborator

thx, well noted

@RobertDober
Copy link
Collaborator

I did some research here are three possible behaviors.

  1. wrap a li's content into a p if there is a space before or after the item.
    (many implementations do that)
  2. Wrap withp in case any line has a space before or after the item.
    (some, and amongst them commonmark, which is a surprise to me, do that)
  3. For minimal backward incompatibility wrap with p if an item is followed by an empty line or the last line is preceded by an empty line. N.B. That implies a p for the last but one item too.

I like 2. best for it's simplicity, and because that is what we have right now on master. However it might be the most violent concerning backward incompatibility.

RobertDober added a commit that referenced this issue Oct 10, 2016
@RobertDober
Copy link
Collaborator

@pragdave I guess we have enough in the 1.0.2 milestone to release. Do you agree? Thank you in advance.

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

No branches or pull requests

3 participants