Add click through functionality for KolodaView

[ferencbeutel4711]

• We wanted to display a message below the lowest card of the stack of cards, therefore we needed to be able to force a click to go through it
• Furthermore we added the ability to add/remove the View to the NotificationCenter
• In addition to that we needed the ability to chance all of the default values to our liking, implemented the missing fields in KolodaView

[AEugene]

What is the purpose of these methods?

[ferencbeutel4711]

Hi, if I understand the Notification pattern correctly its every classes responsibility to subscribe/ unsubscribe from the notification center, especially since you dont always know when a class is deinitialized. E.g: You create a new View in a Controller and subscribe the view to a specific notification. Now you let swift handle the deinitialization since you dont want to be responsible for correctly destroying the UIView.
If you send a notification through the notification center now to an instance which is registered but destroyed already by swift bad things would happen. Thats why you should always implement the deinit method since it is always okay to unsubscribe from the notification center when deinit gets called in the lifecycle (even if you havent even subscribed).
For subscribing, I understand that the method might not be neccessary since you could add the instance to the notification center from the initializing controller for example. In my understanding it is bad practice though, since subscribing/ unsubscribing is logic which the view itself should include (think of the views state which gets effected). So you ask the View to subscribe itself in the controller and now the view does what it has to do to update its state correctly (think syncing over multiple instances; logging; etc.).
When I look at the subscription I see that there should be more parameters though, that is my bad. The notification center and Notification itself should be parameters too, I will change the PR soon.

@lukas2 Anythnig to add?

[lukas2]

Hm. I think you are not calling subscribeForNotifications anywhere and it can probably be deleted.
You are right that if we subscribed to notifications (which we don't) then we would need to unsubscribe at deinit (or earlier).

Also I think UIOrientationDidChangeNotification may be outdated.

Better to use viewWillTransitionToSize(:withTransitionCoordinator:) or willTransitionToTraitCollection(:withTransitionCoordinator:) because the notion of Portrait / Landscape seems to be going away. (Size classes are more reliable, because the user may be holding the device in landscape-orientation, but because he is using split screen, UI is in fact vertical.)

[AEugene]

@lukas2 You're right. I used notification centre in first versions of Koloda, when there was an issue with layoutSubviews. Now layout works fine and it's called when you change orientation. So I don't see any useful examples where we can use NotificationCentre now.

[ferencbeutel4711]

We do indeed use the subscription, in our controller to be exact :) I added the Notification as an extra parameter so it should be fine.
EDIT: we can remove the notification subscription and handle it in our controller if you do not want to support it in your View. If you want i can remove it and update the PR

[AEugene]

@ferencbeutel I think you should remove it.

[AEugene]

This constants are used in frameForCardAtIndex as default implementation. So it wasn't meant to be adjustable, because user of Koloda can apply totally different pattern for frames calculation and we'll end up with 4 extra variables.

[ferencbeutel4711]

Hi,
sorry it took me so long to respond, university got in the way.
I have removed the notification center subscription/ unsubscription. We do need a way to modify the margin/scaling of the background cards though in order to apply a proper styling. What would be your desired way of achieving that? If you just dont want 4 additional variables maybe it would be a better option to optionally supply a Config object which contains the configuration of all of the variables which would lead to just one additional variable. But I dont really think that that would be a better way tbh.

[AEugene]

Hi @ferencbeutel . I'll include configuration in next version for Swift 3, but for now could you please remove these variables and achieve desired behaviour through subclassing?

[ferencbeutel4711]

Hey @AEugene, that are some good news! I have removed the variables and implemented the desired behavior in our code. Anything more you want me to change?

[AEugene]

hi @ferencbeutel . Everything is good. Thank you for contribution. I'll update pod version in a few days.