SteffeyDev / react-native-popover-view

A well-tested, adaptable, lightweight <Popover> component for react-native
MIT License
614 stars 92 forks source link

Added onOpen prop #39

Closed devlinb closed 5 years ago

devlinb commented 5 years ago

onOpen makes chaining a series of popovers super easy, even simple scenarios like having a popover close itself after a period of time becomes easy to do.

I think this it the right place to make the change, and it works for the simple use cases I've tried.

SteffeyDev commented 5 years ago

Thanks for sending in a request!

This will trigger the onOpen before the animation completes. An argument could be made for calling onOpen after the in animation is complete, what do you think of this difference? Do you want to describe the use cases you have in mind in more detail?

I do see the parallel with onClose which is called before the animation, maybe this calls for a parallel doneOpeningCallback for completion?

SteffeyDev commented 5 years ago

This kinda makes me want to rename things (even though it would be breaking). The onClose prop is really confusing because it isn't called when the popover closes, but instead lets you know that you should close the popover.

Maybe a sequence of callbacks like this would make more sense:

devlinb commented 5 years ago

I am trying to think of something that could reasonably be done in response to onCloseStart or onOpenStart AFAIK the opening and closing animations are going to do their thing no matter what, so being informed about them doesn't do much good, although I guess other effects could be kicked off in response, especially if someone has put a custom animation in place for the opening and closing.

Same sort of idea around onOpenComplete, though again, I can envision some sort of fancy effect kicked off because the popover has finished showing itself.

onRequestClose is IMHO a better name for the user activated close than the current 'onClose'.

I added onOpen so I could chain together a bunch of tutorial popovers without relying on manual timing to cycle through my tutorial, which can be iffy on lower spec phones.

My usage basically looks like

isVisible={this.state.flagPopoverVisible}
onOpen={() => {
    this.popoverTimer = setTimeout(() => this.setState({ flagPopoverVisible: false }), 2000);
}}
onClose={this.closePopoverAndCancelTimer.bind(this, { flagPopoverVisible: false })}
doneClosingCallback={() => { this.startNextTutorialStage({ chatPopoverVisible: true }); }}

(with apologizes for arrow functions being used in render! :-D)

with a similar pattern repeated for each of my popovers, only the state being set changes.

I was sad that as far as I can tell, just changing the fromView doesn't make the popover move itself to the new location. I think some of the functionality for that is already there, that'd be a bit more invasive of a change! (It'd look super cool though!)

(aside: I really would like my current mess of code abstracted out a bit into a nice little sequential popover component, so next I'm going to try toggling isVisible, changing fromView and some other props, turning isVisible on again, and see if the popover appears in the right place! )

SteffeyDev commented 5 years ago

Ok, I released version 2.0.0 with the 5 props mentioned above. Make sure to check the readme cause I introduced some other breaking changes as well. I will close this pull request, thank you for your help! You inspired me to get back to work on the library.

I agree that changing the fromView shouldn't work, can you open a new issue to address that specifically? Just so that I can track it. In the meantime, you could probably use fromRect if you are willing to do a bit more work, changing fromRect should cause the view to shift (though I haven't tested it so I am curious). Or maybe your idea of closing and opening again will work fine (the code is so big I forget exactly how all of it works now).

devlinb commented 5 years ago

I gave up on fromRect, writing a state machine to juggle multiple popovers was easy enough, especially with onOpen! :-D

Thanks for the new version!