dimkr / tootik

A federated nanoblogging service with a Gemini frontend.
gemini://hd.206267.xyz
Apache License 2.0
95 stars 3 forks source link

Change ToHTML to also do one br per n #34

Closed snan closed 6 months ago

dimkr commented 6 months ago

Interesting, I prefer to wait with this until I hear complaints about weird formatting of outgoing posts. From what I see, Mastodon posts (by far the most common kind of posts) only use p tags and if I have to mimic the behavior of some other server, I choose Mastodon.

snan commented 6 months ago

I had missed to para wrap the tests. Also I didn't see this in time before I sent that.

lol, wait, I want to write tests that check for the desired output first, let them fail, then adjust the code until they pass - every time I change one line in your PR one test passes but another starts failing

Writing tests before having grokked consistent semantics can lead to issues with tests contradicting each other.

The PR at 88a39ed preserves newlines from gem to HTML with a mix of paras and breaks.


foo

bar

baz
qux

becomes:

<p><br/><br/><br/><br/>foo<br/><br/><br/></p><p>bar</p><p>baz<br/>qux<br/><br/><br/><br/></p>

The two last newlines in an "inner" sequence of newlines become a paragraph separator sequence, all other newlines become br, and then wrap the whole thing in a pair of paragraph tags.

I thought that was what you wanted: preserve newlines.

The PR at a7b8ecb collapses newlines but doesn't join paras.

So that becomes:

<p>foo</p><p>bar</p><p>baz<br/>qux</p>
dimkr commented 6 months ago

All tests pass except those with leading line breaks.

The output is not faithful HTML representation of the input (I changed the expected output so the tests pass):

func TestToHTML_LeadingLineBreak(t *testing.T) {
    post := "\nthis is a line\nthis is another line"
    expected := `<p>this is a line<br/>this is another line</p>`

    html := ToHTML(post)
    assert.Equal(t, expected, html)
}

func TestToHTML_LeadingLineBreaks(t *testing.T) {
    post := "\n\n\nthis is a line\nthis is another line"
    expected := `<p>this is a line<br/>this is another line</p>`

    html := ToHTML(post)
    assert.Equal(t, expected, html)
}

EDIT: fixed, see snan/tootik#1

snan commented 6 months ago

With the a7b8ecb commit I changed it to collapse white space because I thought you thought it was ugly otherwise 🤷🏻‍♀️

If you want to preserve the leading line breaks and such then revert a7b8ecb. I'll do that if you ask—I'm learning my lesson to not second guess things now so I'm not messing with it otherwise 🤷🏻‍♀️

dimkr commented 6 months ago

I think I'm happy with snan/tootik#1, the way it is now. Text delimited by multiple line breaks becomes a series of paragraphs, single line breaks become <br/> and both leading and trailing line breaks are preserved. The result HTML is visually consistent with the plain input, minus the collapsed line breaks between paragraphs (that's what Mastodon does).

dimkr commented 6 months ago

Squashed together with snan/tootik#1 into 4d07c98. Thank you!