Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

fix: remove snapshot view on controller dismissal #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timglorioso
Copy link
Contributor

These changes are identical to #8, just using a topic branch as per best practices. Fixes #5.

Worth noting you can witness #5 in action in the README gif when the slider values flash after card dismissal!

Thanks again for such an awesome package ❤️

@timglorioso timglorioso changed the title remove snapshot view on controller dismissal fix: remove snapshot view on controller dismissal Dec 2, 2017
Copy link
Contributor

@victorBaro victorBaro left a comment

Choose a reason for hiding this comment

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

Take a look at my comments. Thank you very much for spotting this bug and submitting a PR.

@@ -133,6 +133,16 @@ public class CardStackController: UIViewController {
imageView.pinEdgesToSuperviewEdges()
}

public override func viewWillDisappear(_ animated: Bool) {
super.viewWillDisappear(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I think I was just mimicking the approach for viewWillAppear(_:) without giving it too much thought 😅. Thinking we should just pass along the given animated value.


if let imageView = view.subviews[0] as? UIImageView {
imageView.removeFromSuperview()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a big fan of this casting and accessing the subviews array, it could change in the future. It might be better to keep a reference to the snapshot and remove it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I'll push an alternate solution soon.

@timglorioso
Copy link
Contributor Author

Got around to cleaning this up, let me know what you think.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants