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

getTextDimensions should automatically infer numberOfLines if maxTextWidth is provided #2824

Merged
merged 7 commits into from Aug 10, 2020

Conversation

Nnoerregaard
Copy link
Contributor

Extend getTextDimensions such that a maxTextWidth can optionally be passed along and if it is, it is used to infer the number of lines

…ically be inferred based on that and the text width
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.

The getTextDimensions method should behave like the text method:

  • The option should be named maxWidth instead of maxTextLength
  • It should also work if an array is passed. Maybe we should simply call splitTextToSize and calculate the text dimensions on the result. See jspdf.js#L3541

@Nnoerregaard
Copy link
Contributor Author

Good idea to make it consistent with the text method! I've changed the name of the configuration.
As for array support, that should already be implemented. If you pass along an array and the maxWidth option, the width of the widest text entry will be used as width. If you pass along an array but not the maxWidth option, the number of entries in the array = the number of lines.
If you have a different kind of array support in mind, please elaborate and I'll be happy to implement that instead :)

@themacboy
Copy link

Sorry long time without checking this library.

one question ... if I pass a single (and large line) and maxWidth, How can I get the resulting number of lines? And the change in the height, to adjust the position of next text block. Thx in advance

@HackbrettXXX
Copy link
Collaborator

Currently, if an array is passed, the amountOfLines value will be the one of the last entry in the array. Also, Math.cell is undefined, you probably mean ceil.

I'm getting more and more convinced, that we don't actually need this functionality, since the user can simply call splitTextToSize before calling getTextDimensions. No need to bloat the API here.

@HackbrettXXX
Copy link
Collaborator

@themacboy splitTextToSize is the right answer here, too. ;)

@Nnoerregaard
Copy link
Contributor Author

@HackbrettXXX, you're right! The code for determining number of lines should be moved outside of the for loop determining the width. That's a bug.
With regards to using splitTextToSize, I've implemented that for text strings and arrays in my latest commit.
Regarding your comment about having the user call splitTextToSize themselves, I'd say that I personally don't think that should be the user's responsibility. The method is called getTextDimensions, but it does not return the correct text dimensions if the text is more than one line wide. To require the end user of this method to call splitTextToSize before calling getTextDimensions would force the end user to think about the fact that the length of the array passed along to getTextDimensions translates to the number of lines in the text internally in the method.
Also, the end user would be forced to figure out him/herself how to go from a text string and a maxWidth to an array with the correct number of entries. As you stated, this can be done by the jsPDF method splitTextToSize, but the user might not know about that (I didn't until you mentioned it) and thus have to spend time finding it. Instead, putting the call directly in getTextDimensions would allow the end-user to simply ask for (and get the correct) dimensions for a text string and a specific width without having to worry about how the result is achieved.
@themacboy, the width of any lines of text is determined by tempWidth = this.getStringUnitWidth(text[i], { font: font }) * fontSize;. As you can see a couple of lines above, a single string is converted to an array with only one entry and treated just like an array of strings (text = Array.isArray(text) ? text : [text];)

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.

Ok, you convinced me. maxWidth is a nice convenience feature.

src/modules/cell.js Outdated Show resolved Hide resolved
@@ -192,15 +192,18 @@
);
}

text = Array.isArray(text) ? text : [text];
const maxWidthIsDefind = options.maxWidth !== undefined && options.maxWidth !== null
text = Array.isArray(text) ? text : maxWidthIsDefind ? this.splitTextToSize(text, options.maxWidth) : [text];
Copy link
Collaborator

Choose a reason for hiding this comment

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

This behaves now slightly different than in the text method if an array is passed. We could discuss what's best here, but for the moment it should behave the same. See jspdf.js#L3527.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@HackbrettXXX I just pushed a new commit which should make it behave exactly the same as in the text method

@HackbrettXXX
Copy link
Collaborator

Thanks for the PR and for incorporating the changes I suggested :)

@HackbrettXXX HackbrettXXX merged commit 511d4f6 into parallax:master Aug 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants