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

Replace tokens by splitting text runs/paragraphs instead of replacing #62

Merged
merged 9 commits into from
Jul 19, 2024

Conversation

jensschuppe
Copy link
Contributor

@jensschuppe jensschuppe commented Apr 23, 2024

systopia-reference: 23739

Ad the 1.0 milestone: Let's consider this a bugfix for merging it during the beta phase.

This is still missing some things from what @dontub and @jensschuppe planned to implement:

  • For token contents which imply a new paragraph be created, and are inside of fields or hyperlinks:
    • remove Field elements
    • duplicate Hyperlink elements

Note

I used a document with Live Snippets (their tokens being pre-/suffixed with text, i.e. in the middle of a paragraph) which got those values:

  • This is just some text. - should be a single Text element and get inline-inserted without styles
  • <p>This is a new paragraph</p> - should be a new paragraph (splitting the paragraph containing the token)
  • This is some text with a <strong>bold</strong> part. - should be three Text elements with styles (bold font) and get inline-inserted within the same paragraph
  • <p>This is a new paragraph with a <strong>bold</strong> part.</p> - should be a new TextRun element, creating a new paragraph (splitting the paragraph containing the token)

Note: This changes the code style of the entire TemplateProcessor.php file to comply with CiviCRM coding standards, so it's best reviewed with whitespace changes hidden.

This is also being provided as a PR to PhpWord: PHPOffice/PHPWord/pull/2607.

@jensschuppe jensschuppe added bug Something isn't working status:needs review Code needs review and testing labels Apr 23, 2024
@jensschuppe jensschuppe added this to the 1.0 milestone Apr 23, 2024
@jensschuppe jensschuppe requested a review from dontub April 23, 2024 11:41
@jensschuppe jensschuppe force-pushed the htmlTokens branch 7 times, most recently from e2525b6 to 137b892 Compare April 29, 2024 11:05
@jensschuppe
Copy link
Contributor Author

I added a dowhile loop around the setValue() and setElementsValue() methods, since PhpWord only replaces the first occurrence of a macro variable.

@jensschuppe jensschuppe marked this pull request as ready for review June 6, 2024 07:29
@jensschuppe
Copy link
Contributor Author

@pfigel as discussed, happy to receive a review on that.

Copy link

@pfigel pfigel left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! A lot more scenarios now work out of the box. I found a few that don't fully behave as expected (though the behaviour is still less broken than it was before). I've attached corresponding docx files for each issue, as well as the template.

  1. <strong>bold</strong> - text is printed, but it's not bold. (seems to only happen if the token starts with a tag)
  2. <p>This is a new paragraph</p> - produces an invalid docx; LibreOffice says SAXException: [word/document.xml line 5]: Opening and ending tag mismatch: r line 4 and pPr
  3. normal <strong>bold</strong> - bold styling is applied, but font-family (only) for the bold text is lost

I'm running unoconv 0.9.0, LibreOffice 7.6.4.1 and Civi master.

@dontub dontub mentioned this pull request Jun 18, 2024
@dontub
Copy link
Contributor

dontub commented Jun 18, 2024

@pfigel Can you check if #66 fixes all of your use cases?

@dontub
Copy link
Contributor

dontub commented Jul 11, 2024

@pfigel Can you check if #66 fixes all of your use cases?

Ping @pfigel

@pfigel
Copy link

pfigel commented Jul 15, 2024

@dontub Sorry for the delay! I re-tested the test cases above as well as some more complex templates we use in production. All scenarios work as expected now, so this LGTM. Thanks!

@jensschuppe jensschuppe added status:reviewed and tested Code has received thorough review and test and is ready to be committed/merged and removed status:needs review Code needs review and testing labels Jul 19, 2024
@jensschuppe jensschuppe merged commit 3513c98 into master Jul 19, 2024
10 checks passed
@jensschuppe jensschuppe deleted the htmlTokens branch July 19, 2024 08:17
@jensschuppe jensschuppe added status:fixed The issue has been resolved (usually by committing/merging code) and removed status:reviewed and tested Code has received thorough review and test and is ready to be committed/merged labels Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working status:fixed The issue has been resolved (usually by committing/merging code)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants