add Path2D #2013
add Path2D #2013
Conversation
…llow async getContent() * ## Excalidraw and SVG * 2022-04-16 - @thfrei * * Known issues: * - excalidraw-to-svg (node.js) does not render any hand drawn (freedraw) paths. There is an issue with * Path2D object not present in node-canvas library used by jsdom. (See Trilium PR for samples and other issues * in respective library. Link will be added later). Related links: * - Automattic/node-canvas#2013 * - https://github.com/google/canvas-5-polyfill * - Automattic/node-canvas#1116 * - https://www.npmjs.com/package/path2d-polyfill * - excalidraw-to-svg (node.js) takes quite some time to load an image (1-2s) * - excalidraw-utils (browser) does render freedraw, however NOT freedraw with background * * Due to this issues, we opt to use **only excalidraw in the frontend**. Upon saving, we will also get the SVG * output from the live excalidraw instance. We will save this **SVG side by side the native excalidraw format * in the trilium note**. * * Pro: we will combat bit-rot. Showing the SVG will be very fast, since it is already rendered. * Con: The note will get bigger (maybe +30%?), we will generate more bandwith. * (However, using trilium desktop instance, does not care too much about bandwidth. Size increase is probably * acceptable, as a trade off.)
…llow async getContent() * ## Excalidraw and SVG * 2022-04-16 - @thfrei * * Known issues: * - excalidraw-to-svg (node.js) does not render any hand drawn (freedraw) paths. There is an issue with * Path2D object not present in node-canvas library used by jsdom. (See Trilium PR for samples and other issues * in respective library. Link will be added later). Related links: * - Automattic/node-canvas#2013 * - https://github.com/google/canvas-5-polyfill * - Automattic/node-canvas#1116 * - https://www.npmjs.com/package/path2d-polyfill * - excalidraw-to-svg (node.js) takes quite some time to load an image (1-2s) * - excalidraw-utils (browser) does render freedraw, however NOT freedraw with background * * Due to this issues, we opt to use **only excalidraw in the frontend**. Upon saving, we will also get the SVG * output from the live excalidraw instance. We will save this **SVG side by side the native excalidraw format * in the trilium note**. * * Pro: we will combat bit-rot. Showing the SVG will be very fast, since it is already rendered. * Con: The note will get bigger (maybe +30%?), we will generate more bandwith. * (However, using trilium desktop instance, does not care too much about bandwidth. Size increase is probably * acceptable, as a trade off.)
There was a problem hiding this comment.
Nice start, thanks for working on this! It looks like there's some more work left to make this fully implement the Path2D spec. I pointed a few missing parts out below. Do you want to/have time to work on it?
We have the node-gfx organization where I think the actual Path2D class could live btw. (I'd like to move the DOMMatrix class there also.) I'm happy to work on the C++ changes so that we don't have to shim the context prototype methods like this also.
Surprisingly the web-platform-tests don't seem to really cover Path2D aside from isPointInPath. I was hoping those could save us time writing tests.
| } | ||
|
|
||
| class Path2D { | ||
| segments = []; |
There was a problem hiding this comment.
Our package.json says we support back to Node v6, which means we can't use class fields unfortunately (12+).
| class Path2D { | ||
| segments = []; | ||
| constructor(path) { | ||
| this.addPath(path); |
There was a problem hiding this comment.
I think this needs to be guarded with if (path && typeof path === "string" || path instanceof Path2D), and parse the path if it's a string.
| constructor(path) { | ||
| this.addPath(path); | ||
| } | ||
| addPath(path) { |
There was a problem hiding this comment.
Missing the transform argument.
| this.addPath(path); | ||
| } | ||
| addPath(path) { | ||
| if (path && path instanceof Path2D) { |
There was a problem hiding this comment.
Missing the code to handle this.
| addPath(path) { | ||
| if (path && path instanceof Path2D) { | ||
| } else if (path) { | ||
| this.segments = parse(path); |
There was a problem hiding this comment.
This should append to this's segments instead of replacing them.
|
For the record @zbjornson, the majority of code in this PR looks like it's directly from: https://github.com/nilzona/path2d-polyfill |
zbjornson
force-pushed
the
master
branch
2 times, most recently
from
64ed3d8
to
ff0f2ab
Compare
Thanks for contributing!
---> no