-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add fonts with different font-weights #3036
Conversation
What I mean is that this should still work:
So we need to check if the 4th parameter is an encoding string (one of the four constants) or a fontWeight string (a number or a string like '500' or 'bold'). Regarding this:
AFAIK, the (old) |
src/jspdf.js
Outdated
throw new Error("Invalid Combination of fontweight and fontstyle"); | ||
} | ||
if (fontWeight && fontStyle !== fontWeight) { | ||
fontStyle = fontWeight == 400 ? 'normal' : fontWeight == 700 ? 'bold' : fontStyle + ' ' + fontWeight; |
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.
- make sure to also compare with 'normal' and 'bold' strings as
fontWeight
values. - Maybe add a comment that the
==
vs===
is intentional
My bad i meant to make the |
src/jspdf.js
Outdated
fontWeight, | ||
encoding | ||
) { | ||
let encodingOptions = [ |
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.
Can't use let
. We need to write es5 code.
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.
Done.
src/jspdf.js
Outdated
//IE 11 fix | ||
encoding = arguments[3]; | ||
} else if (arguments[3] && encodingOptions.indexOf(arguments[3]) == -1) { | ||
//if weired combination of fontweight and font style throw error |
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.
typo ;)
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.
Done.
src/jspdf.js
Outdated
(fontStyle == "normal" && fontWeight == "bold") || | ||
(fontStyle == "bold" && fontWeight == "normal") || | ||
(fontStyle == "bold" && fontWeight == 400) || | ||
(fontStyle == "normal" && fontWeight == 700) |
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.
normal+700 as well
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.
Done.
src/jspdf.js
Outdated
fontStyle = | ||
fontWeight == 400 | ||
? "normal" | ||
: fontWeight == 700 | ||
? "bold" | ||
: fontStyle + "" + fontWeight; |
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.
What about "italic" fontStyle? E.g. "italic" + "400" should result in "italic".
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.
Done.
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.
We also need to change the signatures in the types/index.d.ts
file.
src/jspdf.js
Outdated
@@ -4795,7 +4795,8 @@ function jsPDF(options) { | |||
* @memberof jsPDF# | |||
* @name setFont | |||
*/ | |||
API.setFont = function(fontName, fontStyle) { | |||
API.setFont = function(fontName, fontStyle, fontWeight) { | |||
fontStyle = !fontWeight ? fontStyle : fontStyle + "" + fontWeight; |
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.
we need the same logic as in addFont
here.
src/jspdf.js
Outdated
@@ -4850,14 +4851,52 @@ function jsPDF(options) { | |||
* @param {string} postScriptName PDF specification full name for the font. | |||
* @param {string} id PDF-document-instance-specific label assinged to the font. | |||
* @param {string} fontStyle Style of the Font. | |||
* @param {number || string} fontWeight Weight of the Font. |
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.
number|string
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.
Alright, looks good to me now. Please run prettier again, and I'll merge it.
@HackbrettXXX added prettier run already. |
@HackbrettXXX build is failing on some wired issue in all PR. |
Closing: #2920
Now user can pass font-weight to differentiate the style using the following API.
JSPDF.addFont(postScriptName, fontName, fontStyle, fontWeight, encoding)
User can also be able to pass without a fontWeight and encoding.
JSPDF.addFont(postScriptName, fontName, fontStyle)
User can also only pass encoding using like this
JSPDF.addFont(postScriptName, fontName, null, encoding)
Need to discuss on this last case @HackbrettXXX
Need more input on
Make sure that passing an encoding without fontWeight will still work.