search

jodhman / react-eyedrop

Seamlessly integrate a static typed, fully-tested color-picking React component/hook!
24 stars 11 forks source link

No way to remove listener when using once={false} #6

Closed ConorKelleher closed 5 years ago

ConorKelleher commented 5 years ago

I appreciate you adding the optional event listener argument once, as our designer wants our eye dropper tool to allow continuous selection, without disabling the tool, however it seems that the current implementation isn't usable.

The biggest issue is that the component has no logic for removing the click listener it adds, and relies on the listener being automatically removed once the click is completed. However, if you pass the optional argument once={false}, this tells the listener not to remove the handler after the first call (as documented here https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Parameters). This means that if I set this prop to false, then there is no way at all to remove the event listener, so any further clicks in the browser will call the change handler and this will stack if you click the button again, which is a clear data-leak, as well as being a very broken UI.

The second, smaller issue is that the body style is set to cursorInactive whenever a selection is made, regardless of whether or not the user has opted to have once={false}. This means that, while subsequent clicks will trigger another onChange call, the cursor will not reflect that behaviour and will instead always go back to the specified inactive state, despite still being active.

Great job on this though, it's really helped me out having this publicly available

jodhman commented 5 years ago

Hey Conor! First of all, I'm very happy to see that you've found this library useful. Your feedback is very helpful, and I will make sure to adress both of the issues later today. Just need to catch my breath from this 16 hour flight. I'll keep you posted! :-)

jodhman commented 5 years ago

Good day Conor! I believe both the mentioned issues should be resolved now.

Documentation regarding the prop once has been updated. Essentially, if once is set to false, and then later on changed to true, the event listener will be removed from the document & the cursor will return to cursorInactive. Monkey-pressing the button no longer creates multiple event listeners, and if property once is false the body style will not change the cursor to cursorInactive when picking colors.

Please let me know if you run into any issues with this. If not, I wish you a super-pleasant day! :-)

ConorKelleher commented 5 years ago

Hey, thanks for sorting that out so quickly!

I pulled the code and got it to suit my use-case perfectly 👍 I'd be happy to see a new release so I can properly set up the dependency on the new code.

Thanks again - Conor

jodhman commented 5 years ago

Awesome to hear it worked! Just published the latest version on npm(@3.1.1 as of this post). Best of luck in your endeavors! :smile:

ConorKelleher commented 5 years ago

Hey, just updated the dependency and the changes don't seem to be there. I notice in your gitub repo that the dist folder hasn't been updated in 6 months (only the src has), and it seems that that's the one npm is distributing. So I don't think any changes were actually made on the npm side of things

jodhman commented 5 years ago

Ah! Nice catch. Should probably keep some dev notes for personal use. Updated with a new dist. Go with version @3.3.1!