efremidze / Haptica

Easy Haptic Feedback Generator 📳
MIT License
770 stars 42 forks source link

Public API improvements #8

Closed genkernel closed 6 years ago

genkernel commented 6 years ago

There is no real issue and this topic is more of an observation type.

Q.1: I used Haptica as:

button.isHaptic = true // required call.
button.hapticType = .impact(.light) // optional, but has to be set.
button.hapticControlEvents = .touchUpInside // optional, default is .touchDown as per implementation.

My initialization logic has a flaw since button.isHaptic = true immediately triggers a call to addTarget() with .touchDown as default value (if hapticControlEvents is not set). Hence hapticControlEvents should be initialized before button.isHaptic = true invocation to make any effect.

Would this make sense to refactor and introduce methods like these:

func addHaptic(_ haptic: Haptic, forControlEvents events: UIControl.Event = .touchUpInside)
func removeHaptic()

to enable 1 line setup? Methods naming strategy 'addHaptic', 'removeHaptic' stolen from 'addTarget', 'removeTarget'.

Q.2: Does it make sense to implement Hapticable for UIControl rather than UIButton? (since addTarget and removeTargets methods implemented in UIControl). This would enable Haptics for elements like UISwitch.

efremidze commented 6 years ago

Thanks for suggestions. I've made the changes but I haven't deprecated the old implementation yet. Release 2.0.5 will be available soon.

genkernel commented 6 years ago

Hey, thank you for adding this functionality!

Maybe consider not deprecating property-based setup and just support both styles in favor of all other devs who use library.

Quick note, I updated my code and found that when I call addHaptic(...), method does not set 'hapticType'. Thus it does not enable hapticable behavior.

efremidze commented 6 years ago

Fixed 🙂