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

Fix newlines consistency, list item trims and overriding list rendering #27

Merged
merged 4 commits into from
Nov 12, 2016

Conversation

loklaan
Copy link
Collaborator

@loklaan loklaan commented Oct 28, 2016

Hello again,

Following things were fixed, and that could mean this is a major semver change:

  • Block sections (paragraph, code, blockquote, list, headings) would sometimes have different vertical padding (new lines)
  • There was a space before list items
  • Missing distinction between rendering lists and list items, overridable through options
    • listitem is still just a chalk reset
    • list (new option) takes the function (body, ordered) signature. it is responsible for attaching points onto list item

I've introduced a weird padding behaviour; there will always be a newline at the start of the output.

I needed to introduce it to make consistent vertical padding AND make sure that nested lists work. I don't really want it there, but couldn't think of a way to get rid of it.

I'll need you to weigh in with this, concerning the newline-at-the-start-of-output and also how these changes may require a major semver change due to how old expected behaviours have changed.

Lochlan Bunn added 2 commits October 28, 2016 20:01
The newlines for blocks, like paragraphs/code/blockquote/table,
are now consistent; one before and one after.

Other fixes:

* Remove space before list items
* Change default tab size to 4 spaces
* Change how lists are rendered/overriden by options
  * Change 'listitem' option to only affect individual items rather
    than a whole list
  * Add 'list' option for overriding the bullets/numbering behaviour
  * Support nested lists
  * Fix newline differences between ul's and ol's
* Add end to end test, which is a document of full markdown syntax

Possible issues:

Newline at the beginning of output becomes a weird requirement...

A 'block' should have a newline before and after. To achieve this,
we can do that literally, or apply a two newlines before or after;
ideally a two newlines after, to avoid a newline at the start of
the rendered output.

This opportunity breaks down, however, because of how lists are
rendered ontop of each other recursively. They require a newline
before their individual outputs as to avoid rendering into the
same line between list nest levels.
@loklaan
Copy link
Collaborator Author

loklaan commented Oct 28, 2016

Also #hacktoberfest because I want that free shirt, Mmmmm!

@mikaelbr
Copy link
Owner

Oh. How have I not seen that hacktoberfest is an actual organized thing before now. I've seen it mentioned several placed, but never looked into it. August/September/October are traditionally very hectic times for me, so probably for the better I didn't get another thing to try to achieve 😛

@loklaan
Copy link
Collaborator Author

loklaan commented Oct 28, 2016

Dat free shirt tho 😻

@hackergrrl
Copy link

Awesome work @loklaan! I tried some of my presentation md files and noticed sublists acting a bit odd:

# test

hello there

1. one
2. two
 - bar
3. three

appears as

# test

hello there

    1. one
    2. two
    3.     * bar
    4. three

on master it renders as:

# test
hello there



    1. one
    2. two
       3. bar
    4. three

Which also seems incorrect. I'd expect it to render just the input (an ordered list with an unordered sublist).

@loklaan
Copy link
Collaborator Author

loklaan commented Oct 28, 2016

Great catch @noffle! Fixed and added tests.

@mikaelbr
Copy link
Owner

mikaelbr commented Nov 1, 2016

Back from a conference! I'll remind myself to check this out tonight.

@mikaelbr
Copy link
Owner

mikaelbr commented Nov 1, 2016

So, looking into this, the prefixed newline for it is due to all sections starting with \n instead of seperating different sections to have newline in the end? Before, things as headers and paragrafs didn't start with a newline. What do we get by doing this, other than convenient coding?

I'm very skeptical of the leading whitespace, I must say.

@loklaan
Copy link
Collaborator Author

loklaan commented Nov 1, 2016

I'd prefer to have a double newline after the sections, but then nested lists would break. They require a newline before their individual outputs as to avoid rendering into the same line between list nest levels.

Because there isn't (correct me if I am wrong) a way for the blocks to know about the previously rendered block, achieving consistency means accommodating the lowest denominator (nested list support) with a single approach.

}

function section (text) {
return '\n' + text + '\n';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you change this to return text + '\n\n'; you will see that everything works, the newline at the start of output is gone, but nested lists's first item will be merged into the last item of it's parent list.

In doing this, a fix for the first nested list point needs to
be applied to all lists.
@loklaan
Copy link
Collaborator Author

loklaan commented Nov 12, 2016

OK 👏
COOL 👏
DONE 👏

Leading newline has been removed. I think the PR is ready ready.

@mikaelbr
Copy link
Owner

Great work! Looking good. 👏 🎉

@mikaelbr mikaelbr merged commit 01e8042 into master Nov 12, 2016
@loklaan loklaan deleted the fix-newlines branch November 12, 2016 10:16
@loklaan
Copy link
Collaborator Author

loklaan commented Nov 12, 2016

Since the output looks different, in terms of newlines/whitespace, should this be a major or minor?

@mikaelbr
Copy link
Owner

Really unsure about this, if the output is a part of the API or not, but I guess there's no reason not to do a major release.

@mikaelbr
Copy link
Owner

There's also breaking changes in the listitem behaviour.

@mikaelbr
Copy link
Owner

New version out: 2.0.0

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

Successfully merging this pull request may close these issues.

3 participants