Skip to content

Conversation

@igorkulman
Copy link
Contributor

This PR adds a few improvements I came up using the side menu in my project

  • delegate method to know when the menu is animating: useful to update status bar appearance when the menu is dark and the VC light or vice versa
  • property to hide / disable the menu completely: useful when you want to have the menu enabled in your root VC but not in any pushed VCs
  • exposing a property to know if the side menu is opened or not when not using the custom navigation controller

Copy link
Owner

@evnaz evnaz left a comment

Choose a reason for hiding this comment

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

@igorkulman Thanks for the contribution. I find these changes useful. Though I have concerns regarding calling a delegate method inside an animation block. Please see my comment for a possible solution.

withDuration: animationDuration,
animations: { [weak self] () -> Void in
self?.sideMenuContainerView.frame = destFrame
self?.delegate?.sideMenuIsAnimating()
Copy link
Owner

Choose a reason for hiding this comment

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

I anticipate some unexpected side effects on the use site if the delegate method would be called inside an animation block. Instead I would suggest introducing another two delegate callbacks with an isAnimated argument and call them before scheduling the animation:
sideMenuWillOpen:isAnimated
sideMenuWillClose:isAnimated
I see this as a more universal approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a good point, but my specific case requries the delegate call inside the animate method so the statubar changes gradually not at once wzen animation startes or finishes.

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.

2 participants