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 event for didHide #29

Closed
wants to merge 4 commits into from
Closed

Add event for didHide #29

wants to merge 4 commits into from

Conversation

enahum
Copy link
Collaborator

@enahum enahum commented Jun 28, 2017

This PR introduces an event handler for when the tooltip hides

@enahum enahum mentioned this pull request Jun 28, 2017
ToolTip.ios.js Outdated
};
},

showMenu: function() {
ToolTipMenu.show(findNodeHandle(this.refs.toolTipText), this.getOptionTexts(), this.props.arrowDirection);
this.props.onShow();
Copy link
Owner

Choose a reason for hiding this comment

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

this is superficial and can live in userland

Copy link

@farkob farkob Jun 30, 2017

Choose a reason for hiding this comment

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

This is true for programmatically displaying the tooltip but there's no way to attach an event when you use show it using the longPress prop. I think there should be e.g. if you want to change the style when the tooltip is displayed.

Copy link
Owner

Choose a reason for hiding this comment

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

showMenu here is only used when programmatically displaying the tooltip. about ur usecase:

y can't you do something like this:

handleLongPress() {
  showMenu();
  this.setState({ style: { ...} })
}

that's just an example. I'm trying to understand the use case better.

Copy link

Choose a reason for hiding this comment

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

That's because the either onLongPress or either onPress is overwritten and there is no way to opt-out. So it can't be used like a controlled component. Hope this helps.

ToolTip.ios.js Outdated
arrowDirection: 'down'
arrowDirection: 'down',
onHide: () => true,
onShow: () => true
Copy link
Owner

Choose a reason for hiding this comment

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

remove

ToolTip.ios.js Outdated
@@ -24,15 +24,19 @@ var propTypes = {
var ViewClass = React.createClass({
getDefaultProps: function() {
return {
arrowDirection: 'down'
arrowDirection: 'down',
onHide: () => true,
Copy link
Owner

Choose a reason for hiding this comment

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

don't default here

ToolTip.ios.js Outdated
},
hideMenu: function() {
ToolTipMenu.hide();
this.props.onHide();
Copy link
Owner

Choose a reason for hiding this comment

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

just check if onHide is available

Copy link
Owner

Choose a reason for hiding this comment

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

wondering if this would create a cycle when invoking hideMenu programmatically? something like ->
hideMenu() => tooltipMenu.hide => onHide => onBlur will be called => hideMenu ....

@@ -1,6 +1,7 @@
#import <UIKit/UIKit.h>
#import <React/RCTBridgeModule.h>


Copy link
Owner

Choose a reason for hiding this comment

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

extra newline

@enahum
Copy link
Collaborator Author

enahum commented Jan 24, 2018

@chirag04 @farkob guys let me know if you are ok by taking this, or lets just close it ;)

@chirag04
Copy link
Owner

@enahum unfortunately i'm not using/maintaining this module anymore. guessing @jrichardlai is also not using it anymore.

If this is something you care about and use i'm happy to add you as a collaborator. Let me know if that works for you.

@enahum
Copy link
Collaborator Author

enahum commented Jan 24, 2018

@chirag04 actually we use this module in our production app https://github.com/mattermost/mattermost-mobile and I do have a fork of this project. Let me bring this to discussion in our organization and I’ll get back to you.

@fungilation
Copy link

Second for use in production. I'm using it in http://www.wonderswipe.com/

@chirag04
Copy link
Owner

To be clear, I use this module in production for lrnapp.com. Just that i don't maintain the app and this module anymore.

@fungilation Looks cool.

@enahum
Copy link
Collaborator Author

enahum commented Jan 24, 2018

@chirag04 we discuss this in our org and we would like to collaborate as an org perhaps you could join our mattermost pre-release server and talk about it some more?

Server: https://pre-release.mattermost.com
My username is: elias

Feel free to ping me there if you are interested

@enahum
Copy link
Collaborator Author

enahum commented Feb 8, 2018

will submit another PR for this one.

@enahum enahum closed this Feb 8, 2018
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.

4 participants