-
Notifications
You must be signed in to change notification settings - Fork 6
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
(feat): add built-in option to clone the canvas (w/ support for alternate canvas implementations) #7
base: master
Are you sure you want to change the base?
Conversation
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
add a link to MDN?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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......
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
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
+window
orcypress
ornode-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.