senny / sablon

Ruby Document Template Processor based on docx templates and Mail Merge fields.
MIT License
446 stars 128 forks source link

Support arbitrary node attributes on paragraphs #51

Closed stadelmanma closed 7 years ago

stadelmanma commented 7 years ago

This lays the ground work for support of the style attribute by allowing any attributes set on an HTML node to be passed onto the WordML paragraph node. It also removes the need of the ListParagraph node subclass since we can now just pass the numPr attributes directly through.

Currently all attributes get shoved into the following template <w:%s w:val="%s" />, this will likely change when I implement styles because of tags like </w:b>. Nested attributes can be achieved by storing the value of your attribute in square brackets, i.e. "[ilvl: #{@builder.ilvl}; numId: #{@definition.numid};]".

I didn't intend for this to be advertised as a public API since it is primarily meant to serve our own internal needs but I didn't prevent it by white listing accepted attributes (or clearing them and setting our own). I figure there might be some good use cases out there that i don't know about. Implementing the style= tag will make a pretty large diff in of itself just from the required boiler plate and creation of tests so I decided to split the PRs up again.

(This PR conflicts with #49 in two files so which ever one you merge first, I'll fix the other)

stadelmanma commented 7 years ago

@senny I have changed the previous logic using the string pattern match for nesting attributes to use nested Arrays/Hashes instead. While the other approach seemed pretty slick since it worked with the native Nokogiri node attributes my revised method is much more robust, and logical. Instead of storing the properties as attributes on the node I am instead directly passing a properties hash into Paragraph.initialize, similar to where you were passing the style directly in before.

The previous syntax was: 'numPr' => "[ilvl: #{@builder.ilvl}; numId: #{@definition.numid};]" And has been replaced by:

{
  'numPr' => [
    { 'ilvl' => @builder.ilvl }, { 'numId' => @definition.numid }
  ]
}
stadelmanma commented 7 years ago

To clarify things, since I forgot to strike out the first half of that comment as well. No node attributes passed onto the WordXML node anymore except the pStyle attribute which we set based on the HTML tag. In the case of lists I pass the numPr props shown above, the use of properties still removes the need for a ListParagraph class.

stadelmanma commented 7 years ago

@senny I have added the properties class but in favor of doing things in a more flexible manner I called it NodeProperties instead. When calling the to_docx method on the properties instance you pass in the base XML tag name (w:p for paragraphs) to get the proper markup for that specific node.

I implemented a [](key) and []=(key,value) method for convenient access to the underlying hash since that is the only data it will ever store.

There is now a robust test case in converter_test.rb that throughly checks the logic and the recursive nature of transform_attr.

stadelmanma commented 7 years ago

@senny, updated to_docx call signature to match. I changed the NodePoperties.initialize method to accept the required tagname. Instead of implicitly appending a 'Pr' to the parent tag I thought it might be better to be explicit with things and pass the tagname in as a whole (i.e 'pPr' instead)

Subclassing an abstract NodeProperties would seem like overkill to just change the tagname used.

stadelmanma commented 7 years ago

Requested changes added. The only place w:pPr was getting used outside of tests was inside the Paragraph class itself. I added a test for the NodeProperties.paragraph method but I didn't convert all of the other unit tests over to use it. I didn't see a need to route all of our unit tests through the factory when the underlying code is 'tagname' agnostic and we want to maintain that behavoir for future expansion.

senny commented 7 years ago

thank you.