-
Notifications
You must be signed in to change notification settings - Fork 191
Conversation
Anyone to merge this PR? |
lib/FullScreenContainer.js
Outdated
|
||
// action behaviour must be implemented by the client | ||
// so, call the client method or simply ignore this event | ||
if (undefined !== this.props.onPhotoLongPress) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about defining a default noop onPhotoLongPress prop instead of undefined checking? Like onPhotoLongPress: () => {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied your previous code.
lib/FullScreenContainer.js
Outdated
<TouchableWithoutFeedback | ||
onPress={this._toggleControls} | ||
onLongPress={this._onPhotoLongPressed} | ||
delayLongPress={this.props.delayLongPress || 1000}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another default prop value would be better here.
Hey @XadillaX. Thanks for your time! I pointed two simple matters and the PR seems fine otherwise. I can merge this right away if you can take a look at them. |
aa64fef
to
ce4343a
Compare
I've updated the code. |
You can review the new code now. |
Thanks @XadillaX, it's merged now. |
Image this situation: someone want to long press the photo to save it.