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 "block" option/action to markdownify #3040

Closed
alizain opened this Issue Feb 15, 2017 · 17 comments

Comments

Projects
None yet
7 participants
@alizain

alizain commented Feb 15, 2017

As @robsonsobral commented in the original issue #1025 that added this "functionality", this is really non-standard, confusing behaviour. Why was it added in? More importantly, can it be removed, or put behind a config option?

For context, refer to this commit be627fa. Hugo manually strips the leading and trailing paragraph tags, thereby rendering the markdownify function useless for any string that contains block level markdown.

This can cause major bugs! For example, if your yaml frontmatter looks like this:

somekey: |
  This is a **valid** markdown paragraph.

  This is another **valid** markdown paragraph. And it's *still valid yaml!*
someotherkey: 2

markdownify will output the following, incorrect HTML, where the opening tag for the first paragraph, and closing tag for the last paragraph are missing!

This is a <strong>valid</strong> markdown paragraph.</p>
<p>This is another <strong>valid</strong> markdown paragraph. And it's <em>still valid yaml!</em>

@bep bep changed the title from Why remove the <p> produced by markdownify!? to Do not remove paragraph tags produced by markdownify Feb 15, 2017

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Feb 15, 2017

Member

You are right, that was a bad idea -- I'm not sure what I was thinking.

Member

bep commented Feb 15, 2017

You are right, that was a bad idea -- I'm not sure what I was thinking.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Feb 15, 2017

Member

Or, thinking about it -- I'm not sure. The main use case for markdownify is for titles etc. and not blocks of test, and it is easier to add p tags than it is to remove it from a template.

Member

bep commented Feb 15, 2017

Or, thinking about it -- I'm not sure. The main use case for markdownify is for titles etc. and not blocks of test, and it is easier to add p tags than it is to remove it from a template.

@robsonsobral

This comment has been minimized.

Show comment
Hide comment
@robsonsobral

robsonsobral Feb 15, 2017

Can we have a parameter for that? Something like inlineOnly, which will allow only inline level elements, like b, strong, em, a...

robsonsobral commented Feb 15, 2017

Can we have a parameter for that? Something like inlineOnly, which will allow only inline level elements, like b, strong, em, a...

@robsonsobral

This comment has been minimized.

Show comment
Hide comment
@robsonsobral

robsonsobral Feb 15, 2017

it is easier to add p tags than it is to remove it from a template

Not always.

robsonsobral commented Feb 15, 2017

it is easier to add p tags than it is to remove it from a template

Not always.

@alizain

This comment has been minimized.

Show comment
Hide comment
@alizain

alizain Feb 15, 2017

and it is easier to add p tags than it is to remove it from a template.

It's actually a lot harder. Especially when you have non-<p> elements that begin or end the string.

alizain commented Feb 15, 2017

and it is easier to add p tags than it is to remove it from a template.

It's actually a lot harder. Especially when you have non-<p> elements that begin or end the string.

@alizain

This comment has been minimized.

Show comment
Hide comment
@alizain

alizain Feb 15, 2017

Perhaps markdownifyBlock & markdownifyInline functions? As mentioned, I see the use-case for inline markdown styles in headers, but calling the function markdown and then producing non-standard output is highly confusing.

alizain commented Feb 15, 2017

Perhaps markdownifyBlock & markdownifyInline functions? As mentioned, I see the use-case for inline markdown styles in headers, but calling the function markdown and then producing non-standard output is highly confusing.

@bep bep changed the title from Do not remove paragraph tags produced by markdownify to Add "block" option/action to markdownify Feb 16, 2017

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Feb 16, 2017

Member

Here is what we need:

  1. markdownify must continue to work as inline
  2. We need a way to signal a block option/action
  3. Even if we have variants like this in partialCached, I don't like that very much.
  4. markdownify "block" isn't very pretty, either
  5. I have thought about adding some kind of namespace into the template funcs (strings.trimLeft etc.) that could fit into this, i.e. markdownify.block

Not sure if the . is allowed there, but something similar ...

Thoughts?

@moorereason @digitalcraftsman @spf13

Member

bep commented Feb 16, 2017

Here is what we need:

  1. markdownify must continue to work as inline
  2. We need a way to signal a block option/action
  3. Even if we have variants like this in partialCached, I don't like that very much.
  4. markdownify "block" isn't very pretty, either
  5. I have thought about adding some kind of namespace into the template funcs (strings.trimLeft etc.) that could fit into this, i.e. markdownify.block

Not sure if the . is allowed there, but something similar ...

Thoughts?

@moorereason @digitalcraftsman @spf13

@robsonsobral

This comment has been minimized.

Show comment
Hide comment
@robsonsobral

robsonsobral Feb 16, 2017

markdownify standard? I don't know.

Non standards behaviors should die in fire. Hugo is young yet. I think it's still time to deprecate markdownify and create a new standard function. With a bad name, of course. "markdownify" is a great name. markdownIt? toMarkdown?

Thank you all for your attention.

robsonsobral commented Feb 16, 2017

markdownify standard? I don't know.

Non standards behaviors should die in fire. Hugo is young yet. I think it's still time to deprecate markdownify and create a new standard function. With a bad name, of course. "markdownify" is a great name. markdownIt? toMarkdown?

Thank you all for your attention.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Feb 16, 2017

Member

Non standards behaviors should die in fire

What standard? The markdownify standard???

Member

bep commented Feb 16, 2017

Non standards behaviors should die in fire

What standard? The markdownify standard???

@robsonsobral

This comment has been minimized.

Show comment
Hide comment
@robsonsobral

robsonsobral Feb 16, 2017

Yeap. Which is the same as the Markdown Extra and others markdown libs. To remove the p tags is a non standard behavior.

Sometimes we need to output just inline markdown, but this case is the exception, not the default.

robsonsobral commented Feb 16, 2017

Yeap. Which is the same as the Markdown Extra and others markdown libs. To remove the p tags is a non standard behavior.

Sometimes we need to output just inline markdown, but this case is the exception, not the default.

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Feb 16, 2017

Member

No, that is not a standard. The markdownify standard becomes whatever WE document that it does.

Member

bep commented Feb 16, 2017

No, that is not a standard. The markdownify standard becomes whatever WE document that it does.

@yesh

This comment has been minimized.

Show comment
Hide comment
@yesh

yesh Apr 20, 2017

is this issue still without solution?

I'm using multiline toml syntax to create little blocks of content that needs to be markdownified and I'm encountering issues with the first paragraph not wrapped in <p>.

I think the main goal of markdownify is to add the correct html tags to build up a content block.
maybe the solution could be to create a new function called something like beautify that does the things markdownify does today, and roll back markdownify to the original behaviour.

@bep what do you think? is this issue currently being considered or is abandoned?

thank you!

yesh commented Apr 20, 2017

is this issue still without solution?

I'm using multiline toml syntax to create little blocks of content that needs to be markdownified and I'm encountering issues with the first paragraph not wrapped in <p>.

I think the main goal of markdownify is to add the correct html tags to build up a content block.
maybe the solution could be to create a new function called something like beautify that does the things markdownify does today, and roll back markdownify to the original behaviour.

@bep what do you think? is this issue currently being considered or is abandoned?

thank you!

@bep

This comment has been minimized.

Show comment
Hide comment
@bep

bep Apr 20, 2017

Member

@bep what do you think? is this issue currently being considered or is abandoned?

It is not closed, so it's not abandoned.

Member

bep commented Apr 20, 2017

@bep what do you think? is this issue currently being considered or is abandoned?

It is not closed, so it's not abandoned.

@bep bep added the Enhancement label Aug 8, 2017

@bep bep added this to the v0.27 milestone Aug 8, 2017

@bep bep self-assigned this Aug 8, 2017

bep added a commit to bep/hugo that referenced this issue Aug 8, 2017

tpl: Add strings.Blocks
To mark som text as one or more blocks. Note that the `block` keyword is reserved in Go templates, hence this plural.

See #3040

bep added a commit to bep/hugo that referenced this issue Aug 8, 2017

tpl/transform: Avoid removing p tags in markdownify when marked as bl…
…ocks

As in the example below:

```
{{ "Blocks of *text*." | blocks | markdownify}}
```

Fixes #3040

bep added a commit to bep/hugo that referenced this issue Aug 8, 2017

tpl: Add strings.Blocks
To mark som text as one or more blocks. Note that the `block` keyword is reserved in Go templates, hence this plural.

See #3040

bep added a commit to bep/hugo that referenced this issue Aug 8, 2017

tpl/transform: Avoid removing p tags in markdownify when marked as bl…
…ocks

As in the example below:

```
{{ "Blocks of *text*." | blocks | markdownify}}
```

Fixes #3040

bep added a commit to bep/hugo that referenced this issue Aug 9, 2017

@bep bep closed this in #3789 Aug 10, 2017

bep added a commit that referenced this issue Aug 10, 2017

@noonat

This comment has been minimized.

Show comment
Hide comment
@noonat

noonat Nov 19, 2017

This makes things a little difficult -- in the past, one could always wrap the transform (e.g. <p>{{ x | markdownify }}</p>) if you wanted it to appear as a paragraph, and it would work with a single paragraph or with multiple.

Now, depending on whether the transformed text has multiple paragraphs, it ends up as <p>...</p> or <p><p>...</p><p>...</p></p>, and I can't figure out a way to fix that from within a template. Any suggestions on a workaround?

noonat commented Nov 19, 2017

This makes things a little difficult -- in the past, one could always wrap the transform (e.g. <p>{{ x | markdownify }}</p>) if you wanted it to appear as a paragraph, and it would work with a single paragraph or with multiple.

Now, depending on whether the transformed text has multiple paragraphs, it ends up as <p>...</p> or <p><p>...</p><p>...</p></p>, and I can't figure out a way to fix that from within a template. Any suggestions on a workaround?

@robsonsobral

This comment has been minimized.

Show comment
Hide comment
@robsonsobral

robsonsobral Nov 19, 2017

@noonat , I'm sorry. I don't understand you. What happened this time?

robsonsobral commented Nov 19, 2017

@noonat , I'm sorry. I don't understand you. What happened this time?

@imjasonmiller

This comment has been minimized.

Show comment
Hide comment
@imjasonmiller

imjasonmiller Dec 16, 2017

I am not sure if I understand you correctly @noonat, but @bep posted something similar somewhere else, and I used that in order to always wrap my text in either a single <p> tag or multiple:

{{ $markdown := .intro | markdownify }}

{{ if not ( strings.Contains $markdown "<p>" ) }}
    <p>{{ $markdown }}</p>
{{ else }}
    {{ $markdown }}
{{ end }}

Just replace .intro with your own.

imjasonmiller commented Dec 16, 2017

I am not sure if I understand you correctly @noonat, but @bep posted something similar somewhere else, and I used that in order to always wrap my text in either a single <p> tag or multiple:

{{ $markdown := .intro | markdownify }}

{{ if not ( strings.Contains $markdown "<p>" ) }}
    <p>{{ $markdown }}</p>
{{ else }}
    {{ $markdown }}
{{ end }}

Just replace .intro with your own.

@ben-turner

This comment has been minimized.

Show comment
Hide comment
@ben-turner

ben-turner Aug 21, 2018

Contributor

I hate to necro this issue back from the dead, but the provided solution doesn't work in all cases. For example, if my markdown is # only a heading, this will produce

<p>
  <h1 id="only-a-heading">only a heading</h1>
</p>

Which is not at all what one would generally expect, and is quite difficult to try to fix.

Edit: It also isn't valid HTML since a paragraph element cannot contain a heading element.

Contributor

ben-turner commented Aug 21, 2018

I hate to necro this issue back from the dead, but the provided solution doesn't work in all cases. For example, if my markdown is # only a heading, this will produce

<p>
  <h1 id="only-a-heading">only a heading</h1>
</p>

Which is not at all what one would generally expect, and is quite difficult to try to fix.

Edit: It also isn't valid HTML since a paragraph element cannot contain a heading element.

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