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

Improved nl2br #557

Merged
merged 3 commits into from Jul 3, 2018

Conversation

Projects
None yet
2 participants
@stephanbrunker
Copy link

stephanbrunker commented Jul 1, 2018

Because of a lack of time, it took a while, but I had an improvement to make which extends the one I did end of last year. Basically, the plugin uses now a much cleaner html with simple

tags. Only where no margin top and bottom is wanted (that means between body and extended article), special classes are used. The second improvement is that single newlines convert to
tags. Additionally, i made a lot of fixes in the tag_clean and explode_along_tags function so that all non-valid < and > characters are escaped. Also the newline converting is only made where the paragraphs are allowed by html. The basic functionality is unchanged so existing articles don't have to be changed. Simple put: you can enter simple text, and the plugin does the formatting for you, or you spice it up with as many html as you want and the plugin only works in the unformatted parts.

Stephan Brunker added some commits Dec 27, 2017

Stephan Brunker
improved nl2br to make it more intelligent
tag clean recognizes quotes in tags and escapes all non-tags
bugfixes in explode along tags
uses now simple <p>-tags normally and only in special cases margin=0
converts double newlines to paragraphs and single newlines to br-tags
but only inside text, not in tags and only where it is allowed by html standards

@onli onli merged commit e09b260 into s9y:master Jul 3, 2018

@onli

This comment has been minimized.

Copy link
Member

onli commented Jul 3, 2018

Okay. I'm looking forward to see this in action, thus I merged it right now to increase the chances this gets tested a lot. Like last time: Thanks a lot :)

@stephanbrunker

This comment has been minimized.

Copy link
Author

stephanbrunker commented Jul 3, 2018

I used it in my own blog first and saw no obvious issues. But if something rises up, I can fix that quickly. Normally, users think in ways unimaginable to the developers ....

@onli

This comment has been minimized.

Copy link
Member

onli commented Feb 22, 2019

Hi @stephanbrunker, there seem to be some issues in this code. I'm testing the current git master in my blog right now and have a look at https://www.onli-blogging.de/1496/Prismatik-unter-Ubuntu-mit-Hyperion-ersetzen.html: all the code blocks do not have my pre class="code" anymore wrapping them, they get replaced by ps dropped. I confirmed that nl2br is the issue here.

I also do not like that there now is always whitespace between the last paragraph and the footer, that would have been a p class="linebreak" before and not introduce that whitespace.

Could you have a look at this and try to fix this?

@stephanbrunker

This comment has been minimized.

Copy link
Author

stephanbrunker commented Feb 22, 2019

@onli

This comment has been minimized.

Copy link
Member

onli commented Feb 22, 2019

Hi
I had html tags in my first version of this comment and did not realize Github would not encode it.

The logic of the nl2p mode was:

  1. If a block of text is followed by one \nl, wrap it in p class="linebreak"
  2. If a block of text is followed by two or more \nl, wrap it in p class="whitespace"

I'll first test your patch now (thanks!) and think about the smaller issue later, though I really see a problem with the current situation, as now an update to the alpha changes how blog articles look. Maybe we should have the plugin emit the CSS to fix this and then it's okay, but the pre tag is more important so far.

@stephanbrunker

This comment has been minimized.

Copy link
Author

stephanbrunker commented Feb 22, 2019

I thought about how the plugin should work, with the intent to produce a well-written html out of plain text with some html in it. And my solution was: Change simple line breaks into br-tags and double breaks into p-tags. I think in the first run, I only improved the existing logic, but after I added more and more intelligence to it, it seemed like a dead end regarding the possibilities which elements are valid parents for p-tags and so on. Then I dropped these classes and changed it. But it shouldn't change the general look of an article: there is still only a line break or a 1em margin. The only conflicting point is the last line of the article and there it is more a matter of personal opinion and that should be addressed by the author in the CSS or by the template, because that much depends on which elements are next below the article.

@onli

This comment has been minimized.

Copy link
Member

onli commented Feb 22, 2019

The only conflicting point is the last line of the article

Right, apart from that it works really well :)

that should be addressed by the author in the CSS or by the template, because that much depends on which elements are next below the article.

I think it should be fixed in the plugin, but we can fix it with CSS emitted by the plugin. The themes are already built without that additional margin in mind, for entries and comments. A user would have a blog running as he wants, update to the new s9y version and then have to change its theme to react to this change in nl2br. I'll have a try with the CSS, your idea above with .serendipity_entry_body p:last-of-type could work fine.

@onli

This comment has been minimized.

Copy link
Member

onli commented Mar 8, 2019

Hi. I found a new related bug. Please have a look at https://www.onli-blogging.de/242/Toshiba-Satellite-4030-CDT-mit-Linux.html. The article looks fine just until first the <strike>. Afterwards all the text is transformed to be all lower case and the html is not interpreted anymore. In my tests it looks like the nl2br plugin is responsible here, deactivating it fixes the issue. Do you have an idea where the issue is exactly?

@stephanbrunker

This comment has been minimized.

Copy link
Author

stephanbrunker commented Mar 8, 2019

Because strike is not recommended in HTML5, I didn't add it to the list of inline-tags. That one is easy to fix. The bug is that in this case, the plugin should only escape the tags of the strike and not everything else. I'm going to take a look at the escaping feature, it should reset if a valid tag occurs. The idea is that you can use the < > without the need to escape them (like in smaller or bigger as), to do it automatically if they aren't part of a valid html tag.

The issue is in the tag_clean function. I'm looking into it, it is quite difficult to determine what is html and what not if you allow < and > in your text - but it is doable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.