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
547 stars 119 forks source link

Fixed error when drawing single dot. #8

Closed lopis closed 6 years ago

lopis commented 7 years ago

When drawing a single point, the canvas would remain empty and the following error would pop on the console:

TypeError: undefined has no properties[Learn More]
dotsize
index.js:25:6

This PR fixes that issue. I was having a problem with this because the canvas would look empty but the state of the component would contain a blank image.

agilgur5 commented 7 years ago

This change did not make a whole lot of sense (checking existence of this is very strange, and I'm not sure why the default should be 2) but #11 defines what the real problem is that caused this error.

This PR in its current state just hardcodes dotSize to 2. If you'd like to edit this PR to make the changes required by #11 , I can then accept it.

lopis commented 7 years ago

Cool. Sorry for the dirty hack. I think I can spare some time to fix and rewrite this PR tomorrow if no one beats me to it.

lopis commented 7 years ago

@agilgur5 Maybe something like this would work? Please review once more.

kgram commented 7 years ago

@agilgur5, any updates on getting this merged? :)

khrizt commented 7 years ago

@kgram @lopis Looks like this package is not really maintained right now... are you interested in forking it and maintaining it? I could help too.

lopis commented 7 years ago

Sure. We're using this fork with out own changes but it needs some cleaning up. We're going to continue using it and maintaining it for the forseable future.

khrizt commented 7 years ago

Ok, do you need any help? Any plans on publishing it in NPM?

kgram commented 7 years ago

I wouldn't mind pitching in. The problem, as I see it, is that there is already 7 almost identical signature-pad packages for react on npm (react-another-signature-pad even seems to poke fun of this itself). There are differences in API and implementation, but they all basically serve the same purpose.

I think the community would be much better served if this was kept under @agilgur5, possibly with commit-access being granted to someone else if @agilgur5 does not have enough time himself. Alternatively, publishing rights to the current package could be passed on.

The name of this package is good, it has the most downloads and from what I've seen, the best API. Adding an 8th package with a weird variation on the name seems ill conceived. It'll just add confusion and fracture the development effort.

lopis commented 7 years ago

I personally agree with @kgram. It would be hard to gain any traction with a new fork.

khrizt commented 7 years ago

Yeah, I understand that, but there are some important things to resolve like this pull request, so I hope it can be as you say soon.

lopis commented 7 years ago

Unfortunately you'll have to maintain your own repo and cherry pick the things you want and keep rebasing any updates. Not ideal but honestly better than depending on someone else.

agilgur5 commented 6 years ago

This has been rebased and released in v0.2.2 as 342ce9609b990c71ba9fe0cea11a42fe4607ea77 and backported to v0.1.8 as 81e426c13ab7b0483ad7c3dec1b71022919c6240 (as part of the new v0.1.x branch).

I've had quite a hectic past several months that's taken up the majority of my time (winding down my start-up -- for whom I had made this fork, looking for a new beginning, moving across the country, and settling into a new life) and no longer write much of any front-end code (or open up my personal laptop much), so I do apologize for not maintaining this with as much gusto as I previously did. I do get email updates on this repo, but I unfortunately had higher priorities to deal with.

I had been looking to merge this in in August before I moved and subsequently went on hiatus, but got too deep into thinking whether this should be a patch bump or a major bump (this functionality was broken in a previous commit, but the change here is not necessarily backward-compatible with previous code either) and ended up not resolving that. I released it as a patch bump just now because it is a bugfix and the previous functionality was difficult/unintuitive to use (due to usage of this; I am actually unsure what the usage of it was beforehand).

I was also not aware that this was a high priority issue, as an error when drawing a single dot and only then does not seem very impactful on UX or DX at all; I assume this causes some other, more problematic issues not listed here.

I will say that I was not expecting this fork to become this popular nor need much attention, given it's small surface area, but it appears I was at least somewhat wrong. I wish NPM had begun with namespaces originally, allowing forks to live separately easily, but unfortunately naming forks is a thing we must deal with as a community (though at least referencing a GH repository, i.e. a fork, instead of package is relatively easily with NPM). I agree with @kgram and @lopis on their points (better to have one repo and that is what comes of relying on others). I'm not opposed to having someone else with commit access and have accepted most PRs, but as you may have noticed, I try to be rigorous with commit messages, major v. minor v. patch, and backporting, and I am unsure that another maintainer will keep that standard (perhaps a worthwhile sacrifice, perhaps not).