Fixed item listing, moved margin-left to <li> by botzill · Pull Request #225 · CenterForOpenScience/pydocx

botzill · 2017-02-01T16:55:12Z

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 5a4d93a#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 · 2017-02-01T17:08:07Z

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-rldatix · 2017-02-01T17:08:58Z

@winhamwr I think the discussion is in #224

botzill · 2017-02-01T17:11:43Z

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 · 2017-02-01T17:12:26Z

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 · 2017-02-01T18:05:59Z

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 · 2017-02-03T14:06:21Z

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

Thx!

winhamwr · 2017-02-14T18:12:59Z

tests/export/test_xml.py

+                    <li>CCC</li>
+                </ol>
+            </li>
+            <li>DDD</li>


This testcase really helped me understand the new behavior and how it's superior to the previous behavior. Thanks for including it!

winhamwr · 2017-02-14T18:18:16Z

pydocx/export/html.py

+
+        if indentation_first_line:
+            first_line = convert_twips_to_ems(indentation_first_line)
+            # TODO text-indent doesn't work with inline elements like span


Can we remove this TODO, now that we're adding display: inline-block?

winhamwr · 2017-02-14T18:20:11Z

pydocx/export/html.py

+            if prev_level_paragraphs:
+                return prev_level_paragraphs[-1]
+
+            if prev_level_id == 0 and not prev_level_paragraphs:


Do we need the and not prev_level_paragraphs condition? Seems like we'd get the same behavior without it.

winhamwr · 2017-02-14T18:21:00Z

pydocx/export/html.py

+                return prev_level_paragraphs[-1]
+
+            if prev_level_id == 0 and not prev_level_paragraphs:
+                # this is an edge case with older version of word when it may contain a sublist


Thanks for the "why" comment! Can we capitalize the t in This to give it the treatment of a proper english sentence?

winhamwr · 2017-02-14T18:26:45Z

pydocx/export/html.py

-            except ValueError:
-                right = None
+        # for numbering properties we add style to span item level
+        if properties.numbering_properties is None:


How would you feel about switching this conditional so that it was if properties.numbering_properties so that the else case was the 2-line case? Just a matter of preference, but I struggled for a bit reading this until I realized that the first case was essentially the "do nothing fancy" case.

I'd also love a comment explaining the do-something case like Numbering properties can define a text indentation on a paragraph

winhamwr · 2017-02-14T18:28:40Z

pydocx/export/html.py

+
+        return None
+
+    def export_listing_paragraph_property_indentation(self, paragraph, level_properties,


The (undocumented, sorry) coding standard this project follows is to use one line per kwarg when the definition stretches across lines.

It is on a new line, no?

Sorry that I wasn't clear. Like this:

def export_listing_paragraph_property_indentation( self, paragraph, level_properties, include_text_indent=False, ):

Actually, don't worry about these minor style issues, including my notes on comment capitalization/punctuation. I can handle all of that trivial stuff after we get this merged in. We can't expect you to read our mind on those things, since they aren't documented in our coding standards.

Nop, good to know ;).

winhamwr · 2017-02-14T18:32:07Z

pydocx/export/html.py

+        hanging = paragraph_ind_hanging or level_ind_hanging
+
+        # at this point we have no info about indentation, so we keep the default one
+        if not left and not hanging:


Are we ignoring the possibility that only paragraph_ind_first_line is set on purpose? Is that case handled with this line?

Well, if paragraph is part of the listing then it's level will contain the left or hanging. I don't think we can have a listing paragraph where we don't have left/hanging on it's level and on paragraph but first line is set.

winhamwr · 2017-02-14T18:32:46Z

pydocx/export/html.py

+        left -= hanging
+
+        if level_id == 0:
+            # because html ul/ol/li elements have there default indentations


there => their

winhamwr · 2017-02-14T18:33:29Z

pydocx/export/html.py

+
+        if level_id == 0:
+            # because html ul/ol/li elements have there default indentations
+            # we remove the default word one as well


Minor sentence cleanup:
We remove...
...as well.
This way...
...to html.

winhamwr · 2017-02-14T18:34:07Z

pydocx/export/html.py

+            # for nested levels we need to add indentation based on parent level
+            prev_paragraph = self.get_previous_level_paragraph(num_id, level_id)
+            if prev_paragraph:
+                prev_left_level_indentation = prev_paragraph.get_numbering_level().\


We go with the style of opening the parens and then indenting the next line, rather than using \.

like this:

prev_left_level_indentation = prev_paragraph.get_numbering_level( ).paragraph_properties.to_int('indentation_left')

?

winhamwr · 2017-02-14T18:34:44Z

pydocx/export/html.py

+            if hanging is not None:
+                # Now, here we add the hanging as text-indent for the paragraph
+                hanging = convert_twips_to_ems(hanging)
+                # TODO text-indent doesn't work with inline elements like span


Do we want to add the display: inline-block bit here, too?

Well, here we add margins to li elements, so no need for display: inline-block at this stage.

winhamwr · 2017-02-14T18:39:13Z

pydocx/export/numbering_span.py

            return None
        return level

+    def detect_parent_child_map_for_items(self):


@jlward @kylegibson would either of you be willing to give this method a look? I understand what this is supposed to do (thanks to the excellent docstring), and I understand the code (looks good), but I don't understand the interaction between numbering_properties, abstract_num_id, and other concepts here to give it a confident review.

I just realized that this methods needs to be adapted to handle more cases, especially any nested separated list. Will look into it tomorrow and try to adapt for all cases. Any feedback is welcome.

winhamwr · 2017-02-16T23:08:09Z

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 · 2017-02-17T06:00:55Z

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 · 2017-02-17T15:02:33Z

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

winhamwr

Excellent work!

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

5a4d93a

Fixed no 'level_indentation' for fake lists

407a516

botzill force-pushed the fix-list-items-processing branch 6 times, most recently from 29cc421 to 7d2f65f Compare February 3, 2017 14:04

Added test cases for the added features

1bd667e

botzill force-pushed the fix-list-items-processing branch from 7d2f65f to 1bd667e Compare February 4, 2017 09:12

Fixed issue with multiple nested separted lists

97dff05

botzill force-pushed the fix-list-items-processing branch from fde949a to 97dff05 Compare February 7, 2017 06:49

Fixed paragraph text-indent when in span

8acfe02

botzill mentioned this pull request Feb 7, 2017

Paragraph inside listing #227

Open

jhubert mentioned this pull request Feb 9, 2017

Incorrect parsing of list items - missing tabs/spaces. #202

Open

winhamwr reviewed Feb 14, 2017

View reviewed changes

Fixed review remarks

84ab79f

botzill force-pushed the fix-list-items-processing branch 3 times, most recently from 85a56e2 to 7174428 Compare February 16, 2017 18:18

Refactor nested separate lists to include more scenarios

a7b31cc

botzill force-pushed the fix-list-items-processing branch from 7174428 to a7b31cc Compare February 17, 2017 05:55

Fixed margins for separated nested lists

3ae50cb

botzill force-pushed the fix-list-items-processing branch from d48ef1d to 3ae50cb Compare February 17, 2017 17:35

winhamwr approved these changes Feb 17, 2017

View reviewed changes

winhamwr merged commit 98c6aa6 into CenterForOpenScience:master Feb 17, 2017

regneel-TL mentioned this pull request Feb 26, 2024

Revert pydocx commits made after latest stable release PolicyHarbor/pydocx#2

Merged


		return None

		def export_listing_paragraph_property_indentation(self, paragraph, level_properties,

Comments

Conversation

botzill commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

winhamwr commented Feb 1, 2017

Uh oh!

kylegibson-rldatix commented Feb 1, 2017

Uh oh!

botzill commented Feb 1, 2017

Uh oh!

winhamwr commented Feb 1, 2017

Uh oh!

botzill commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

botzill commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botzill Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

botzill Feb 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

winhamwr commented Feb 16, 2017

Uh oh!

botzill commented Feb 17, 2017

Uh oh!

botzill commented Feb 17, 2017

Uh oh!

winhamwr left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

botzill commented Feb 1, 2017 •

edited

Loading

botzill commented Feb 1, 2017 •

edited

Loading

botzill Feb 14, 2017 •

edited

Loading

botzill Feb 14, 2017 •

edited

Loading