-
-
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
chore(TS): config #8194
chore(TS): config #8194
Conversation
Code Coverage Summary
|
Code Coverage Summary
|
1 similar comment
Code Coverage Summary
|
src/config.ts
Outdated
*/ | ||
forceGLPutImageData = false | ||
|
||
NUM_FRACTION_DIGITS = 2 |
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 leave a note here, that this represent the number of digits when exporting to json. Is a good idea to bump this to 4 or more for designs app, and that is kept to 2 otherwise testing the library is a mess
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 think we should remove this strange const and introduce it in the options object passed to toObject
and toSVG
It is really bad IMO, e.g. #8178
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.
And fix tests accordingly
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.
bumped to 4
for me is fine, those things had to go out from header ( probably all header will go away ) |
header go away! |
Code Coverage Summary
|
testem doesn't log errors for some inane reason
This reverts commit 51aef74.
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
Code Coverage Summary
|
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 really dislike NUM_FRATION_DIGITS
and think it should be replaced with an option
But for now I am done
*/ | ||
fabric.minCacheSideLimit = 256; | ||
config.configure({ | ||
devicePixelRatio: fabric.window.devicePixelRatio || |
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.
once the window context is exported we should move this logic to config.ts
* Map of font files | ||
* Map<fontFamily, pathToFile> of font files | ||
*/ | ||
fontPaths: Record</** fontFamily */ string, /** pathToFile */ 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.
I don't really understand for this is used for
Can it be made a local var in canvas#toSVG
?
It's the only place it is used....
I thought exposing registerFonts
that will add fonts to fontPaths
and call register fonts on node canvas.
??
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 can be made local to the svg export function yes. But for this PR we should leave it here i guess.
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.
only used by canvas.toSVG
Most of these values can be localized I guess... |
fabric.window.webkitDevicePixelRatio || | ||
fabric.window.mozDevicePixelRatio || |
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 think we can verify which browser still need those, and we can removed if is something super old.
* @default 1 | ||
*/ | ||
fabric.browserShadowBlurConstant = 1; | ||
fabric.charWidthsCache = {}; |
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 m going to take care of this later for the charWidthsCache. I want to change it to a Map.
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.
excellent
but change it in #8198
this.fontPaths = { | ||
...this.fontPaths, | ||
...paths | ||
}; |
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.
Object.assign(this.fontPaths, paths) in general seems nicer to me, but i don't care.
* @todo remove once rollup supports transforming enums... ANNOYING! | ||
* https://github.com/rollup/plugins/issues/463 | ||
*/ |
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 is the the output so far? isn't TS that supports enum?
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.
rollup isn't doing it right and not building an object out of the enum as TS does
@asturur What do you think about this approach?If you approve I will replace all occurrences of the configured files
Extract config from HEADER to a dedicated class