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

Added new onSwipe callbacks #248

Merged
merged 9 commits into from
Feb 12, 2019
Merged

Added new onSwipe callbacks #248

merged 9 commits into from
Feb 12, 2019

Conversation

Purii
Copy link
Contributor

@Purii Purii commented Feb 7, 2019

Great component!

I needed a method to get notified, once the user started swiping the modal:
Modal from bottom containing an arrow icon. The icon shall be animated based on the swiping state.

Feel free to provide improvements 👍

example

@mmazzarolo
Copy link
Member

@Purii thank you for the PR!
You're right, a few more "swiping" props would be useful.
My main issue with the PR is the naming convention:

  • onSwipe
  • onSwiping
  • onSwipingDone
    They seem a bit confusing now, but that's just because the original onSwipe props should have been named onSwipeDone/onSwipeComplete instead... 🤔

Would it make sense doing something like this (just asking for feedback, I'm not saying you should do it in the PR 😃)?

  • Deprecate the onSwipe callback (but still support it)
  • Create the following callbacks:
    • onSwipeStart
    • onSwipeMove
    • onSwipeComplete
    • onSwipeCancel

What do you think?

@Purii
Copy link
Contributor Author

Purii commented Feb 8, 2019

I'm also not that happy with the naming.
Your suggestion sounds much better. I'm going to update the PR :)

@mmazzarolo
Copy link
Member

@Purii thank you sir 🙇

@Purii
Copy link
Contributor Author

Purii commented Feb 9, 2019

@mmazzarolo You're welcome!
I added the four new methods.

Deprecation onSwipe
The deprecation warning will be triggered on componentDidMount.

Added Prettier Config
During development I saw, that my local prettier config is different than the one used in the repo.
So I added a prettier config to override different local settings.
I also set trailingComma:true.
Hope this is fine for you?

Let me know if I should change something :)

@Purii
Copy link
Contributor Author

Purii commented Feb 11, 2019

Any idea for a better naming of percentageLeft as value of onSwipeMove?

Copy link
Member

@mmazzarolo mmazzarolo left a comment

Choose a reason for hiding this comment

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

Any idea for a better naming of percentageLeft as value of onSwipeMove

swipeProgress maybe? Did you check if it is invoked correctly? (Can't check it right now).

Look good to me, top notch contribution 👌

No worries about the prettier config, we are planning to start using a community driven one so it probably be changed again soon.

console.warn(
'`<Modal onSwipe="..." />` is deprecated. Use `<Modal onSwipeComplete="..." />` instead.',
);
}
Copy link
Member

Choose a reason for hiding this comment

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

❤️

}
if (this.props.scrollTo && this.props.scrollOffset > 0) {
return false; // user needs to be able to scroll content back up
}
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the cleanup

@Purii
Copy link
Contributor Author

Purii commented Feb 11, 2019

swipeProgress maybe? Did you check if it is invoked correctly? (Can't check it right now).

Yep. Will be invoked on each move. No throttling, as I would leave that responsibility to the user.
Currently I just return newOpacityFactor, that's why I labeled it percentageLeft. Wouldn't swipeProgress implicate the progress of the swipe and not the way still to go? But we could give it a try: swipeProgress === newOpacityFactor

@mmazzarolo
Copy link
Member

Wouldn't swipeProgress implicate the progress of the swipe and not the way still to go?

🤦‍♂️ my bad, you're right.
Mind reverting it for the last time? Thank you!

@Purii
Copy link
Contributor Author

Purii commented Feb 12, 2019

Renamed it to percentageShown. Still not perfect, but understandable.
Also updated the example: onSwipe --> onSwipeComplete 👍

@Purii
Copy link
Contributor Author

Purii commented Feb 12, 2019

Is the prettier config fine for you?
Changed some formatting of the example (added trailing comma,...)

@mmazzarolo
Copy link
Member

Is the prettier config fine for you?

Yes, see:

No worries about the prettier config, we are planning to start using a community driven one so it probably be changed again soon.

Merging, thanks @Purii !

@mmazzarolo mmazzarolo merged commit ba26000 into react-native-modal:master Feb 12, 2019
@mmazzarolo mmazzarolo changed the title Added onSwiping method Added new onSwipe callbacks Feb 21, 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