-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(Text): _getFontDeclaration
#9082
Conversation
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.
fixed
Build Stats
|
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
fontFamily.includes(',') || | ||
Text.genericFonts.includes(fontFamily.toLowerCase()) | ||
? fontFamily | ||
: `"${fontFamily}"`; | ||
return [ |
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.
Is this a hot path method? (e.g. called when rendering) In that case you should check if a string interpolation ${fontStyle} ${fontWeight} etc.
is significantly less expensive than creating an array on the fly and doing join
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 thought we should do that. It is called more than once in rendering
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 should recheck our code. We changed so much without measuring, and we don't even have a way of properly measuring.
@@ -2,6 +2,7 @@ | |||
|
|||
## [next] | |||
|
|||
- fix(Text): `_getFontDeclaration` [#9082](https://github.com/fabricjs/fabric.js/pull/9082) |
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.
This changelog doesn't tell much about the change. It should be more _getFontDeclaration: passed style should take precedence
as the test case.
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.
point noted
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.
Yeah i also changed the pr description and the merge commit, but forgot to update the changelog.
Just for the record: _getFontDeclaration() {
let {
fontFamily = this.fontFamily,
fontStyle = this.fontStyle,
fontWeight = this.fontWeight,
fontSize = this.fontSize
} = arguments.length > 0 && arguments[0] !== undefined ? arguments[0] : {};
let forMeasuring = arguments.length > 1 ? arguments[1] : undefined;
const parsedFontFamily = fontFamily.includes("'") || fontFamily.includes('"') || fontFamily.includes(',') || Text.genericFonts.includes(fontFamily.toLowerCase()) ? fontFamily : "\"".concat(fontFamily, "\"");
return [fontStyle, fontWeight, "".concat(forMeasuring ? this.CACHE_FONT_SIZE : fontSize, "px"), parsedFontFamily].join(' ');
} This is how the code gets transpiled so far. |
Seems like no browser supports optional arguments correctly, unless you ask babel to support only chrome 100 onward. that seems very strange to me. |
Motivation
closes #9010
closes #9041
Description
The function _getFontDeclaration was suppose to take the font information from either the style object or the instance itself, but for fontFamily there is a bug in which the fontFamily is only taken from the instance.
Changes
Gist
In Action