Problems if alert is shown on menuItem click (iOS only)

[jenni-divvito]

Hello

Right now there is a problem when showing an alert in the onClick of a MenuItem. This would look to be related to facebook/react-native#10471 and is only for iOS. The long shot is that if an alert is opened while a modal is closing, the alert disappears and the app ui locks up. This persists until the app is closed, so from a user perspective, it is basically a crash.

Seeing as it's a bug in RN, it should be up to them to fix it, but seeing as the issue has been untouched by them for almost 2 years I'm not sure that can be relied upon.

Based on the discussion in the linked issue, it would appear that this is best resolved through the use of the onDismiss() callback from Modal, however that is iOS only. I wasn't really sure the best way to resolve this in the way this library works, seeing as the fix requires the use of a platform specific feature, which is what keeps me from submitting a PR, but in the meantime I am just getting around it through liberal application of setTimeout, however this makes things feel a tad laggy.

I'm not sure the library authored would prefer this to be resolved. If you don't want to put platform specific workaround code in the library (understandable), then it might be best to just expose onDismiss directly on the Menu component? That way users can make use of it external to your library.

I'm happy to throw together a PR when I have the time, but just want to be sure which method the authors would prefer first.

[mxck]

@chris-divvito I check it today.

[mxck]

@chris-divvito some one already add onHidden prop in Menu. You can use it.
But I think best way is add callback in hide method. Example:

  onPress = () => {
    this._menu.hide(() => {
      alert('WOW! MENU IS CLOSED!')
    });
  }

Will this be the solution for you?

UPD: This solution doesn't work. So I just move onHidden to onDismiss call :)
UPD2: Hm. Main question how bind menu item click and onDismiss. By example if you have a lot of menu items but you need call callback only from single one.

[jenni-divvito]

yeah, because of that I feel like the best option would be something similar to what you initially posited.

What about if the hide method returned a Promise? This could be implemented like:

hide = () => {
  return new Promise((resolve) => {
    Animated.timing(this.state.opacityAnimation, {
      toValue: 0,
      duration: ANIMATION_DURATION,
      easing: EASING,
    }).start(() =>
      // Reset state
      this.setState({
        menuState: STATES.HIDDEN,
        menuSizeAnimation: new Animated.ValueXY({ x: 0, y: 0 }),
        opacityAnimation: new Animated.Value(0),
      }, resolve),
    );
  });
};

then the calling code could do something like:

onPress = () => {
  this._menu.hide().then(() => {
    alert('WOW! MENU IS CLOSED!')
  });
}

?

[mxck]

Modal have fade animation, so menu not really hidden after state update. So this method doesn't work for alert bug.

[jenni-divvito]

Oh - my understanding of the CompositeAnimation API was that the callback provided to .start() was called on completion, not initialization. Am I wrong there?

It's poorly documented on RN's side, but looking at it's usage in other places I always took it to be a completion callback, and it's treated that way in some various tutorials and such: https://medium.com/react-native-training/react-native-animations-using-the-animated-api-ebe8e0669fae

[mxck]

Oh my bad. I got a little confused.

https://github.com/mxck/react-native-material-menu/blob/master/src/Menu.js#L108
With this changes alert bug still here.

[jenni-divvito]

Oh it is? hmmmm... this possibly brings me back to my initial suggestion of exposing the onDismiss for <Modal/> - That looks to be how they resolved the same issue over here -> wookoinc/react-native-popover-tooltip#22

I'd be happy to put together a pull request if you'd like

[mxck]

@chris-divvito sorry for late answer. You can make it :)
in https://github.com/mxck/react-native-material-menu/blob/master/src/Menu.js#L110
use something like

if (Platform.OS === 'android' && this.props.onHidden) {
  this.props.onHidden();
}

and call onHidden on modal dismiss.

[geekvijay]

@mxck, I think you can close this issue as we are now calling the onHidden method on Modal onDismiss for ios.

[mxck]

If the error is still present, I will reopen issue.