search

agilgur5 / react-signature-canvas

A React wrapper component around signature_pad (in < 150 LoC). Unopinionated and heavily updated fork of react-signature-pad
https://agilgur5.github.io/react-signature-canvas/
Other
546 stars 119 forks source link

Add new example where canvas is not cleared on resize #9

Closed lopis closed 3 years ago

lopis commented 7 years ago

Picking up the work from https://github.com/agilgur5/react-signature-canvas/pull/2 I cleaned up the PR to only include the relevant changes to this one feature. I also changed the nature of the new prop in that PR to preserve compatilibity.

{/* Clear canvas as usual on window resize (and sometimes on mobile scroll!) */}
<SignaturePad {...rest}/>

{/* Disables clear on resize */}
<SignaturePad clearOnResize={false} {...rest}/>

It might also be possible to prevent the mobile scroll bug by checking if the window height really changed. But that requires a bit more testing.

agilgur5 commented 7 years ago

Thanks for your contribution @lopis , backward compatibility is definitely needed for a feature that may not behave optimally for all use cases.

There's some key changes that I think should be made before this is merged: 1) Naming -- clearOnResize as per #2 is much clearer / explicit wording than keepOnResize (the latter makes me ask "keep what?"). clearOnResize would be better to use, but default it to true. An explicit clearOnResize={false} also would make it more evident that it's not recommended behavior for all use cases.

2) The new prop should be able to be changed. In the current state of this PR, changing it once it has been set initially will have no effect, as the window event handlers will not be changed. This would require either adding a componentWillReceiveProps lifecycle hook, or in a more straightforward manner that matches other changes in the library, only having the prop be checked in _resizeCanvas, causing the function to essentially be void when the canvas should not be cleared (the latter is preferred, to be explicit).

(Btw, this isn't a Dropzone :P . Also an ={true} is redundant in React props)

lopis commented 7 years ago

Oops @Dropzone. Wrong copy paste! I'll look into this and re-commit

agilgur5 commented 7 years ago

The option has been added in 56cf03d4e30a052979e8377cb25d87ab17af470c and published as v0.2.0 (backward compatible minor bump). Thanks for the contribution!!

Not closing this out just yet as the example wasn't rebased as I haven't reviewed, read that in detail, or tried it myself yet. I'm likely going to move the example/ directory into a gh-pages branch so it actually uses the package (as opposed to importing the component), has its own devDependencies, and has a URL to link to.

agilgur5 commented 5 years ago

I still haven't gotten to this in the past few years (😐), but wanted to write down here that there is now a gh-pages branch. The repo link also points to the published example now as well.

Part of the reason in finally making it was to be able to easily reproduce issues (or attempt to), such as https://github.com/agilgur5/trim-canvas/issues/5

Separately, I've gone around to thinking that examples, just like docs, should be part of the main codebase. I like that the gh-pages branch directly imports react-signature-canvas as a package from NPM though, so maybe the ideal is to just keep the gh-pages example in-sync with the main repo. Or something.

Also having two examples might need tabs or a sidebar (like storybook or something). tbd when that'll happen.