Skip to content

Conversation

ST-1580
Copy link
Contributor

@ST-1580 ST-1580 commented Oct 15, 2021

We've fixed this bug

var lastLine = "";
var lineCount = 0;
Line: for (var i in textSplit) {
Line: for (let i = 0; i < textSplit.length; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change let to var. Potentially breaking old JavaScript Engines.


for (var x = firstWordInLine; x <= lastWordInLine; x++) {
line += textSplit[x] + " ";
const currLine = textSplit[x];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please change const to var

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's actually no longer necessary. I've added Babel to the deployment, so you can use modern JS.

Copy link
Collaborator

@HackbrettXXX HackbrettXXX 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 the PR. Please add another test case with longer lines (so the font size gets smaller).

Comment on lines +220 to +224
if (formObject.multiline) {
textSplit = textSplit.map(word => word.split("\n"));
} else {
textSplit = textSplit.map(word => [word]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, this looks a bit fishy to me. Does this work for a text like "abc \n def"? Wouldn't it be better to split first into lines and then into words?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I will add new test soon. Also I reproduced your example and it works:

image

I think that current solve is easier to implement in your code. In my opinion, your implementation will require more new code than my. So it will be harder to find hypothetical new bugs.

@ST-1580 ST-1580 requested a review from HackbrettXXX October 20, 2021 09:49
Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

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

Alright, then we'll keep it this way. Thanks for adding the new test. I'll merge it now.

@garipovroma
Copy link

Awesome pull request guys, thank you all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TextField AcroForm does not render multiline text properly
4 participants