52inc / Pulley

A library to imitate the iOS 10 Maps UI.
https://cocoapods.org/pods/Pulley
MIT License
2.02k stars 265 forks source link

Tapping Dimming View does not collapse drawer when not set as supported position #384

Closed maxmika closed 4 years ago

maxmika commented 4 years ago

I disabled the .collapsed position for my drawerView and when it's in state .open and I click on the remaining space on top of it (good chance of hitting the dimming view) to go back to the (e.g.) map it doesn't collapse – obviously, since I've forbidden that position. It also logs me an error:

"PulleyViewController: You can't set the drawer position to something 
not supported by the current view controller contained in the drawer.
 If you haven't already, you may need to implement the 
PulleyDrawerViewControllerDelegate."

I checked your code and you log that whenever I try to set the drawer to a position that I do not allow. Using the call stack I figured out than whenever the user taps on the dimming view, setDrawerPosition(position: .collapsed, ...) is called. Note the fixed .collapsed parameter, which I didn't choose!

My suggestions:

  1. Let the user either choose to which position the drawer should be set when the dimming view is tapped
  2. Or let the user fully override the "onTap" for the dimming view so he can handle the touch. In my example, I would like to update the content of the drawerView and then collapse it. The new content is also considered by the delegate function supportedDrawerPositions, which would after changing the content also allow .collapsed.

I hope it gets clear what I'm trying to tell you, I think that would be a good improvement.

Thank you 👍

jcbriones commented 4 years ago

I was about to make the same comment and good thing I saw this. .collapsed is not supported on my settings but it's trying to set it as collapse which I didn't choose.

ulmentflam commented 4 years ago

@maxmika and @jcbriones This functionality has been added to the master branch via PR https://github.com/52inc/Pulley/pull/294. I will be pushing a minor point update shortly that includes this PR.

jcbriones commented 4 years ago

Thank you! I appreciate your quick feedback

ulmentflam commented 4 years ago

@jcbriones this has been added in Release 2.8.1!