Fix uinavigationcontrollerdelegate forwarding for real #312
Conversation
acosmicflamingo
force-pushed
the
fix-uinavigationcontrollerdelegate-forwarding-for-real
branch
2 times, most recently
from
68b8583 to
da98da1
Compare
|
Looks like that failure in da98da1 was a false positive. This PR is good for merging then. Edit: pushed final change to remove random .orig file that appeared. |
acosmicflamingo
force-pushed
the
fix-uinavigationcontrollerdelegate-forwarding-for-real
branch
from
91d4943 to
6883e9b
Compare
| reportIssue( | ||
| """ | ||
| UINavigationControllerDelegate object is being set after | ||
| NavigationStackController's viewDidLoad() has been called, | ||
| so delegate functions will not be observed. Set your delegate | ||
| in your subclass's initializer to fix this issue | ||
| """ | ||
| ) |
There was a problem hiding this comment.
Isn't this an issue that plagues UIKit in general? Do we really need to report this issue in our library? People should just know that they always need to set their delegates right when the controller is initialized, and never do it later.
There was a problem hiding this comment.
Oh, I didn't know that 🤣 I was thinking that this could prevent others in the future from creating a ticket in swift-navigation repo. But, I'm happy with whatever decision you make given that I now know what the problem was.
There was a problem hiding this comment.
Yes I don't think we need to report issues that help people use UIKit correctly. If you revert this we will merge and release the PR.
There was a problem hiding this comment.
Sure! I'll do it later tonight when I'm in front of the keyboard.
acosmicflamingo
force-pushed
the
fix-uinavigationcontrollerdelegate-forwarding-for-real
branch
from
6883e9b to
e9a6375
Compare
3 participants
There was lots of discussion in #311 on how to address the issue with
PathDelegatebeing registered as NavigationStackController'ssuper.delegateearlier than expected, so here's a PR that takes @tgrapperon 's changes, adds a purple run-time warning letting users know to set their delegate in theinit, and an update to the test that addresses this issue.