-
Notifications
You must be signed in to change notification settings - Fork 374
Nested html tags use styles of all elements in stack #433
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
base: master
Are you sure you want to change the base?
Conversation
a481950 to
9a3927f
Compare
9a3927f to
bdab62a
Compare
MalteJoe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the COMBINED ContainerType.
Otherwise just some formatting issues.
| */ | ||
| public class HTMLTextStylingContentHandler | ||
| extends DefaultHandler | ||
| public class HTMLTextStylingContentHandler extends DefaultHandler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert the formatting changes in the whole file
| propertiesList.addAll( spansStack ); | ||
| ContainerProperties result = null; | ||
| for(ContainerProperties properties : propertiesList) { | ||
| if (result == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incnsistent formatting (whitespace around braces, curly braces in new line)
| public static ContainerProperties combine( ContainerProperties p1, ContainerProperties p2 ) | ||
| { | ||
| ContainerProperties result = new ContainerProperties( | ||
| p1.getType() == p2.getType() ? p1.getType() : ContainerType.COMBINED ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this implementation lead to problems in ODT when combining Paragraphs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked the ODTDocumentHandler.java currently it doesn't support styles from spans at all. So it really makes no difference. There is probably an Issue or there should be an Issue reading the Span Styles as well and when that is implemented the containerType would be read as well.
|
@MalteJoe what do you think about this PR? |
|
@angelozerr As the Formatting Guidelines mentioned in https://github.com/opensagres/xdocreport/wiki/CodeFormatAndConvention are outdated and may be replaced with https://github.com/palantir/palantir-java-format at some point, I will close my comments regarding formatting. There's still one open comment, but it's too old for me to remember what exactly it was about, as I haven't been working with xdocreport for a while now. |
While working on fixing Issue #432 I noticed a Bug when using nested spans with styles:
This HTML Code should produce a bold and italic text
But because only the first item in the stack is used when converting to docx the result is only italic:
should be: