(feat): add built-in option to clone the canvas (w/ support for alternate canvas implementations) #7
Conversation
There was a problem hiding this comment.
Some changes necessary.
Now that we have tests using jest & node-canvas in #9 (see some rationale and links to rationale in other libraries of mine there about why jest vs. cypress vs. ava), tests for this feature should also be added here.
Also needs to be rebased with current master and again when the tests are merged (which should be soon)
Publish commit should also be updated to my new style.
| - **canvas** *Canvas object* The canvas to be trimmed. | ||
| - **options** *object* Optional arguments. | ||
| - **clone** *bool* Whether to clone the canvas before trimming (default: `false`). | ||
| - **createCanvas** *function* A custom function to create a Canvas (defaults to DOM implementation). |
There was a problem hiding this comment.
should mention that it's only used for cloning as that's a pretty key detail.
Could make clone accept a function instead, but while cleaner and potentially more semantically correct, that seems like an unintuitive API -- even cloneFunc is unintuitive. {clone: {createCanvas: function... }} could make sense but also seems a bit unintuitive and unwieldy... hmmm
| #### `trimCanvas(canvas, options)` | ||
|
|
||
| - arguments | ||
| - **canvas** *Canvas object* The canvas to be trimmed. |
| Supports [`node-canvas`'s `createCanvas`](https://github.com/Automattic/node-canvas#createcanvas). | ||
|
|
||
| - returns the trimmed canvas (original or cloned) | ||
|
|
||
| ## Example | ||
|
|
||
| Can see how `trim-canvas` is used inside of `react-signature-canvas` [here](https://github.com/agilgur5/react-signature-canvas/blob/310bff81813509a4035bedfe50d76e7045a880cb/src/index.js#L53-L64). |
There was a problem hiding this comment.
This link would be out-of-date with this release, as there's no need for react-signature-canvas to implement its own cloning logic after this release. Could link to an unreleased commit that uses the new cloning logic?
index.es6
Outdated
| cropYDiff) | ||
|
|
||
| // early return if cloning | ||
| if (clone) { |
There was a problem hiding this comment.
Per #9 (comment), it probably makes more sense to clone at the top of the function, then the rest of the logic can run uninterrupted on the clone. That would also replicate how the library is used right now without this feature -- one clones the canvas, then calls trimCanvas on the clone.
There was a problem hiding this comment.
The downside is that that method is probably more inefficient, since we'd be copying data into the clone twice, one for cloning and one for trimming. This way it only occurs once, at the expense of somewhat more complicated logic (also that might've been why I did it this way in the first place, but idr). Hmm......
agilgur5
force-pushed
the
clone-option
branch
from
f82ddd4
to
dd42762
Compare
- this is almost always needed by developers, so figured I should add
it into the library itself (well again, but w/o a node-canvas dep)
- while it should probably be the default, will introduce it as
opt-in so it's not a breaking change
- e.g. node-canvas - so this doesn't depend on being run in a browser/DOM context
- show how to clone in Usage and specify that it's v0.2+ - add full API docs - slightly larger than 100 LoC now 😢 - use const instead of let in docs too
- minor bump due to addition of optional arguments `clone` and `createCanvas` - several updates to README included as well
agilgur5
force-pushed
the
clone-option
branch
from
dd42762
to
92a2da5
Compare
Codecov Report
@@ Coverage Diff @@
## master #7 +/- ##
=========================================
- Coverage 100% 85.1% -14.9%
=========================================
Files 1 1
Lines 37 47 +10
Branches 8 11 +3
=========================================
+ Hits 37 40 +3
- Misses 0 7 +7
Continue to review full report at Codecov.
|
(refactor): use const everywhere instead of let …Fixes #6
Looking to add at least one test in before merging.
Not sure if I should use
ava+windoworcypressornode-canvas.I think the ideal test would be to have a fixture of trimmed
ImageData(instantiated probably like this MDN example), insert into a new, larger Canvas, and then trim it and check the trimmed ImageData against the fixture. If it's small enough, hopefully won't take that much code to create an equals check.An alternative, indirect (but simpler) test would be to check that the width and height decreased to the preset/known amount.