DKVerticalColorPicker icon indicating copy to clipboard operation
DKVerticalColorPicker copied to clipboard

support black and white and grayscale between

Open dredenba opened this issue 10 years ago • 4 comments

  • support black and white and grayscale between in addition to color hues
  • add a few simple tests
  • change delegate method to indicate touch type

dredenba avatar Sep 04 '15 18:09 dredenba

Thanks for the contribution, I really do appreciate it, but unfortunately I can't accept this pull request for a few reasons:

  • It crashed for me when testing with EXEC_BAD_ACCESS in the iOS 9 simulator for iPhone 6 under Xcode 7 Beta 6
  • I think the black & white lopped onto the top looks a bit awkward, and should be optional at best
  • I don't understand the use case for the delegate wanting to know the type of touch; it seems to add unnecessary complexity

That said it looks like there are some other nice cleanups in here.

davecom avatar Sep 06 '15 04:09 davecom

Thanks

  1. Don't have the Xcode beta yet, but not sure where the bad access would be coming from based on the code I added since it doesn't add any unusual memory usages ( just a few local variables). Also I would be surprised if any memory issue was introduced by iOS 9 but certainly is possible. Can you explain the steps to reproduce the problem?
  2. not sure if you are suggesting you don't want a black and white added at all? or if you would prefer it look different? or just that you would like the current pull request code to also make the color spectrum configurable?
  3. the added argument on the optional delegate method does serve a purpose (and could be ignored if not needed). For my particular use case the UI would do a different visual transition of the color change when the touch has finished versus during the rapid changes while the user is dragging their finger across the control. This is not unlike having the touch events broken out as the native api does with "touchesBegan", "touchesMoved", and "touchesEnded". The downside of this that I can see is that it means that any codebase using this control would want to update to the new delegate method when they upgrade to the new control code (though this should be a very simple change to do). Another variant on the solution that could provide some backwards compatibility but with the downside of introducing the support "legacy" code would be to have the code check for the original delegate method and use that otherwise use the new delegate method.

Would like to hear your thoughts

dredenba avatar Sep 06 '15 21:09 dredenba

Hi @dredenba

  1. I simply tried running the test project in the iPhone 6 simulator in Xcode 7 Beta 6 and after trying to change the color the EXEC_BAD_ACCESS immediately occurred.
  2. Yeah I don't really like the way it looks - it seems forced rather than a natural transition.
  3. I understand. That's an interesting use case.

davecom avatar Sep 06 '15 22:09 davecom

What do you think about the idea of making the additional grayscale bar optional?

davecom avatar Sep 11 '15 04:09 davecom