Fix issue (#122) withscrollToRevealFirstResponder
#136
Conversation
| @@ -150,33 +150,58 @@ private extension UIScrollView { | |||
| } | |||
| } | |||
|
|
|||
| func adjustContentOffset(_ adjustInsets: (UIView) -> UIEdgeInsets) { | |||
| guard let firstResponder = firstResponder as? UIView else { return } | |||
| func adjustContentOffsetToRevealFirstResponder(_ adjustInsets: (_ firstResponder: UIView) -> UIEdgeInsets) { | |||
There was a problem hiding this comment.
Good renaming, makes the function a lot clearer 👍
| @@ -150,33 +150,58 @@ private extension UIScrollView { | |||
| } | |||
| } | |||
|
|
|||
| func adjustContentOffset(_ adjustInsets: (UIView) -> UIEdgeInsets) { | |||
| guard let firstResponder = firstResponder as? UIView else { return } | |||
| func adjustContentOffsetToRevealFirstResponder(_ adjustInsets: (_ firstResponder: UIView) -> UIEdgeInsets) { | |||
There was a problem hiding this comment.
Nit: I didnt understand the adjustInsets parameter the first time I saw it. Thought it is a bool. Would a renaming make sense here to state it is actually a closure?
There was a problem hiding this comment.
It is part of the public API so I don't feel like changing it in this bug fix PR, it will require a deprecation etc. It has documentation on the public method, I agree that it probably could have been more clear but you know, naming is hard : }
There was a problem hiding this comment.
Ok I see, is fine for me :)
| } | ||
| } | ||
|
|
||
| func targetVisibleRectToRevealFirstResponder(_ adjustInsets: (_ firstResponder: UIView) -> UIEdgeInsets) -> CGRect? { |
There was a problem hiding this comment.
Nit: It is just for discussion. In my previous team we had the rule that every function should start with a verb. I'm used to that and therefore have a hard time reading this function because target is also a verb. In my previous team this would have been getTargetVisibleRectToRevealFirstResponder. If you ask me this makes function calls a lot clearer most of the time. Was something like this discussed earlier here? If everyone is used to it I think I can also get used to it ✌️
There was a problem hiding this comment.
Interesting suggestion, let's discuss this internally
#122
I was struggling to understand why this issue is happening (probably something around the usage of frame vs bounds).. We were getting weird negative offset in few cases causing glitches when switching between fields. We ended up re-writing some of the logic together with @enhorn . Now we're comparing only the vertical positions taking into account scroll view offset and insets.
Will run couple of more manual tests on devices but so far everything looks good. Let me know what do you think.