-
Notifications
You must be signed in to change notification settings - Fork 9
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
Store TeX source #194
Store TeX source #194
Conversation
@@ -237,10 +238,10 @@ More details printed to console.` | |||
return "invalid latex"; | |||
} | |||
|
|||
const img = await this.canvas.addImage( | |||
const img: FabricTeXImage = await this.canvas.addImage( |
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 isn't really necessary (we don't care about how much stronger this type is than Image
, just that it is Image
) but it conveys intent
new Promise<fabric.Image>((resolve) => | ||
options?: T | ||
): Promise<fabric.Image & (typeof options extends undefined ? unknown : T)> => | ||
new Promise((resolve) => |
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 are now allowed to remove the redundant type from the new Promise()
call!
@@ -42,6 +42,7 @@ export default class Pages { | |||
savePage = (): void => { | |||
this.pagesJSON[this.currentIndex] = this.canvas.toObject([ | |||
"id", |
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 (the use of the artificial id
property) should probably be abolished in the future; store in the data
property instead
i've begun to change my mind and am tentatively approving this (sorry for not merging this before #189, but am still thinking about this) |
…opied to the `Object`
9de5292
to
c8c73d7
Compare
That's okay, things should be good now (conflict was due to rebasing #189 on merge, not due to doing it earlier) |
Resolves #193