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

Add onModalWillShow and onModalWillHide callbacks #251

Merged
merged 1 commit into from
Feb 11, 2019
Merged

Add onModalWillShow and onModalWillHide callbacks #251

merged 1 commit into from
Feb 11, 2019

Conversation

cjroth
Copy link
Contributor

@cjroth cjroth commented Feb 10, 2019

Currently there exist onModalShow and onModalHide which are triggered after the modal show and hide animations are complete, but I found it useful to have more granular callback at the beginning of the animations. In my case I was using this for haptic feedback, which didn't feel right after the animation was already complete.

@mmazzarolo
Copy link
Member

@cjroth since this is a controlled component onModalShow and onModalHide shouldn't be needed because you are in control of when you start showing/hiding the modal.

For example, if you wanna log something when you show the modal, instead of doing it in onModalShow you could simply do the following in the parent controller:

showModal = () => {
  this.setState({ isModalVisible: true });
  console.log('Hello world');
}

...or maybe I'm just missing some use cases, let me know!

@cjroth
Copy link
Contributor Author

cjroth commented Feb 11, 2019

That is a totally valid point! Here's my use case:

I added haptic feedback to modal show events. I'm using Redux so isVisible is passed into my component as a prop.

Solution 1: I could add some logic that looks for changes to the isVisible prop. I'd need both previous/next and current props in order to compare them for changes which means I'd have to put the logic in shouldComponentUpdate or componentDidUpdate. This feels like an abuse of React methods.

Solution 2: Keep track of the modal state in the component's local state as well as Redux and look for differences between the two. This adds some complexity and requires me to convert a PureComponent into a Component in order to utilize state.

Solution 3: Add the haptic feedback trigger to my Redux side effects listeners (in my case, sagas). This is the best solution of the three by far, but it still requires me to (A) have a side effects tool and (B) put the logic outside of the component that it is associated with.

My thought is that a modalWillShow and modalWillHide is the most elegant way to achieve this but I also totally support avoiding unnecessary methods if there's a better way or if my use case feels uncommon.

@mmazzarolo
Copy link
Member

Got it. Yeah, this could certainly be helpful if you're encapsulating the modal logic.
Just like you said, I'm not fan of unnecessary props but I don't think these would harm... especially because this question has been already been asked a few times 😬
Thanks for the contribution.

@mmazzarolo mmazzarolo merged commit 409eb9d into react-native-modal:master Feb 11, 2019
@mmazzarolo
Copy link
Member

Landed in v8.0.0-beta.1 🛬

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.

None yet

2 participants