Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send delegate messages for all user-initiated dismissals #137

Merged
merged 12 commits into from
Jan 26, 2016

Conversation

cdzombak
Copy link
Contributor

  • Refactor how dismissals are managed, to allow disambiguating user-initiated and programmatic dismissals for determination of whether to send delegate messages, without tracking an additional bit of state.
  • 16bd3c0 should've been covered in Removed Redundant Swift Tests #128 , but I just now noticed it
  • Other commits in this PR are documentation improvements around this behavior, and minor style improvements.

Closes #134.

…ropriately

This follow-up to #130 fixes a bug wherein user-initiated dismissals
via the Close/X button didn’t send delegate messages about the
dismissal.

closes #134

I would welcome alternative fixes that do not require tracking
additional state.
@cdzombak cdzombak modified the milestone: 1.0.1 Jan 19, 2016
* (an interactive pan or a tap on the Close button).
* Reset to `NO` when an interactive dismissal is not in progress.
*/
@property (nonatomic, getter=isCurrentDismissalUserTriggered) BOOL currentDismissalUserTriggered;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you potentially eliminate this intermediate state if both doneButtonTapped: and didPanWithGestureRecognizer called through to a common method, hypothetically something like dismissViewControllerAnimated:isUserInitiated:completion:, passing YES for isUserInitiated, while dismissViewControllerAnimated:completion: calls through to the common method with NO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bcapps 💯

@soundsokay
Copy link
Contributor

👍 save for the (nice to have) expanded unit test coverage noted in the comment in eff4f08

@cdzombak cdzombak self-assigned this Jan 19, 2016
@cdzombak
Copy link
Contributor Author

screen shot 2016-01-26 at 10 43 07

Currently this test is failing, though setting a breakpoint in -dismissViewControllerAnimated:userInitiated:completion: tells me that that method ​_is_​ being called, with those values (YES, NO, nil). So…what am I missing?

@bcapps
Copy link
Contributor

bcapps commented Jan 26, 2016

Although the code looks right, the error message seems to imply that it's expecting (YES,YES,nil) instead of what you intended.

@cdzombak
Copy link
Contributor Author

Although the code looks right, the error message seems to imply that it's expecting (YES,YES,nil) instead of what you intended.

Hm, I must've skimmed over that. I've been looking at OCMock errors too long this morning.

…blowing away DerivedData doesn't fix this (ie. it's not left from a previously-cached build), so I'm at a bit of a loss.

This makes no difference re: the testing glitch I’m seeing in #137 (comment) .
@cdzombak
Copy link
Contributor Author

922bdb0 makes no difference.

@cdzombak cdzombak assigned soundsokay and unassigned cdzombak Jan 26, 2016
@soundsokay
Copy link
Contributor

👍

soundsokay pushed a commit that referenced this pull request Jan 26, 2016
Send delegate messages for all user-initiated dismissals
@soundsokay soundsokay merged commit fd0ed13 into develop Jan 26, 2016
@soundsokay soundsokay deleted the cdz/134-dismiss-delegates branch January 26, 2016 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants