Skip to content
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

[RFC] Use/wrap upstream signature_pad #17

Closed
agilgur5 opened this issue Jan 18, 2018 · 5 comments
Closed

[RFC] Use/wrap upstream signature_pad #17

agilgur5 opened this issue Jan 18, 2018 · 5 comments
Labels
scope: dependencies Pull requests that update a dependency file

Comments

@agilgur5
Copy link
Owner

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
Copy link
Owner Author

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

@lopis
Copy link

lopis commented Jan 19, 2018

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
Copy link
Owner Author

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
Copy link

kgram commented Jan 22, 2018

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
Copy link
Owner Author

agilgur5 commented Apr 12, 2018

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

Repository owner locked as resolved and limited conversation to collaborators Mar 13, 2021
@agilgur5 agilgur5 linked a pull request Mar 13, 2021 that will close this issue
@agilgur5 agilgur5 added the scope: dependencies Pull requests that update a dependency file label Mar 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
scope: dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants