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

[RFC] Use/wrap upstream signature_pad #17

Closed agilgur5 closed 6 years ago

agilgur5 commented 6 years ago

When I originally forked react-signature-pad, I did not particularly diff the code with the original signature_pad and just made changes downstream as needed. Having looked more closely at signature_pad in recent months (due to the discussion in #8 ), it looks like all of the code is backward-compatible (even the 2.0.0 release is just a dist change which does not break the API).

I propose that this library import signature_pad and effectively just act as a wrapper implementing certain helper functions like trimming and resizing (as it currently does). All props would be passed directly to the signature_pad instance. In order to preserve auto-updating props, a componentWillReceiveProps hook would need to be made that updates the instance's internal variables. I don't think there would be anything backwards-incompatible this way, but I may be wrong (there may be some gimmicks in resize). It may need to pin to patch versions only for now as minor version changes could break the wrapping layer due to internal usage (e.g. opts.ratio, opts.width, opts.height).

This would add a whole host of new functionality, such as adding an SVG option as requested in #12 , as well as fix lots of bugs, such as the isEmpty bug in #16 , and would overall reduce maintenance burden and the general surface area of this library and its API.

agilgur5 commented 6 years ago

@lopis @kgram would love to get your comments on this

lopis commented 6 years ago

I think that's a great idea. I'd need to look into it myself but this would certainly make it easier to maintain the repo. I assumed it was using signature-pad, not rewriting it.

agilgur5 commented 6 years ago

From what I could tell (I didn't do any wholesale diffs as line number and other changes make that difficult), the code looks to have been mostly copied from signature_pad (variable names and style are almost identical, if not identical), which is why I think this could be done without even breaking the API. Part of the reason for #11 , the dotSize bug, was because this doesn't use signature_pad and therefore its initialization was changeable (and was then accidentally broken during the constructor -> static change)

kgram commented 6 years ago

I think it sounds good too. There isn't really all that much gained by maintaining a copy of everything. It'll require some logic for handling reinitialisation since I doubt signature-pad is made to handle changes in cavas size, but it shouldn't be that bad.

agilgur5 commented 6 years ago

Implemented in #20 . Would appreciate any help testing out v1.0.0-alpha.1 in production environments since there might some hidden behaviors or edge cases I missed or something