Fixed item listing, moved margin-left to <li>

botzill commented 7 years ago

No unit tests yet! Will add soon.

About this PR. Basically there are scenarios like in the issue #224 and #202.

Scenario 1 - proper left/right margin:

Currently when we have a doc like:

(old code) will output this:

(new code) will output this:

So, basically proper margins are added to the <li> items like this:

<body><p>Heading 1</p>
<ol class="pydocx-list-style-type-decimal">
    <li style="margin-left:9.00em">AAA</li>
    <li style="margin-left:9.00em">BBB</li>
</ol>
<p>Heading 2</p>
<ul>
    <li style="margin-left:19.50em">CCCC</li>
    <li style="margin-left:19.50em">DDD</li>
</ul>
</body>

Scenario 2 - basically issue from #202:

If we have as input a list like this:

(old code) we get as output this:

(new code) while with new changes we get:

Now, this issues is related to the fact that listings like: B1,B2, C1,C2 are basically independent lists from the main one. This mean that they have a different NumberingProperties and, of course, different numId which is actually breaking the entire listing flow. Currently listing will works if NumberingProperties are the same and level_id is changing(which will cause proper nesting). Now, for this case I did the following(code commends as well https://github.com/CenterForOpenScience/pydocx/pull/225/commits/5a4d93a2d8f11553731119781fe1985b322f9e95#diff-4aa70f6d5310ea0a57e5c20b629dc727R212):

Before we start to process numbering spans/items I read all the listing items(from components) and try to determine such scenarios and create a simple mapping like this:

child_parent_num_map = {
     "<child_num_id>": "<parent_num_id>",
    ....
}
parent_child_num_map = {
     "<parent_num_id>": "<child_num_id>",
    ....
}

Later, when we construct the span/items I do some checks and determine if we should start a new span or we already have a parent one and just add to it. Check PR for more details relate to implementation and let me know what you think about this solution.

Now, this issues was tricky as I needed to understand basically the entire listing flow. Still not sure if I did it properly

winhamwr commented 7 years ago

Hello Chirica,

Thanks for the PR!

There's a lot going on here. It would really help if you could provide both a summary of what was wrong and what the new behavior is, and at least some minimal test coverage demonstrating the fix.

Thanks -Wes

kylegibson commented 7 years ago

@winhamwr I think the discussion is in #224

botzill commented 7 years ago

Ou, I did not expect the PR here :). OK, I will make proper descriptions about it. Still some work in progress here. So, hold on for now.

Thx.

winhamwr commented 7 years ago

Thanks @kylegibson! The discussion there is great.

I guess that means we're just missing a bit of test coverage and a thorough review. I walked through the code itself and it looks high-quality. Thanks for that, Chirica!

So, hold on for now.

Will-do. We'll wait to hear from you before diving in.

botzill commented 7 years ago

OK, I did update the description with what I did so far. Please, if you have time, check the PR and let me know if you have any improvements. I did not covered 100% how listings are get constructed(fake lists as example).

Thx!

botzill commented 7 years ago

@winhamwr @kylegibson any chance you can check the current PR an let me know what you think about it?

Thx!

winhamwr commented 7 years ago

We've given this a couple of days for review, and everything looks very solid to me. @botzill if you can get the python 2.6 tests passing, then I'll merge this in and fix the minor style issues.

botzill commented 7 years ago

Thx @winhamwr, sounds great! I have left some final things to finish related to setting proper margins for those separated nested lists. Hope will not take much time. I want to include that as well in this PR.

p.s Tests fixed.

botzill commented 7 years ago

OK, I think this PR is ready now. Did some changes related to separated nested lists. Now margins are properly set.

CenterForOpenScience / pydocx

Fixed item listing, moved margin-left to <li> #225