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

Store TeX source #194

Merged
merged 2 commits into from
Jun 30, 2021
Merged

Store TeX source #194

merged 2 commits into from
Jun 30, 2021

Conversation

pihart
Copy link
Collaborator

@pihart pihart commented Jun 17, 2021

Resolves #193

@pihart pihart added the priority: high Rush this label Jun 17, 2021
@pihart pihart requested a review from cjquines June 17, 2021 23:46
@@ -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(
Copy link
Collaborator Author

@pihart pihart Jun 17, 2021

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) =>
Copy link
Collaborator Author

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",
Copy link
Collaborator Author

@pihart pihart Jun 17, 2021

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

@pihart pihart mentioned this pull request Jun 18, 2021
@cjquines
Copy link
Owner

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)

@pihart
Copy link
Collaborator Author

pihart commented Jun 20, 2021

That's okay, things should be good now (conflict was due to rebasing #189 on merge, not due to doing it earlier)

@pihart pihart added the semver: minor SemVer represents module (i.e. npm published) compatibility label Jun 20, 2021
@cjquines cjquines merged commit 6605ebb into cjquines:master Jun 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high Rush this semver: minor SemVer represents module (i.e. npm published) compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store source TeX in Image object, serialize
2 participants