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

Make compatible with RN 49+, refactored and added onHide and onShow props #36

Merged
merged 4 commits into from
Feb 8, 2018

Conversation

enahum
Copy link
Collaborator

@enahum enahum commented Feb 8, 2018

  • Refactored to be compatible with RN 49+
  • Added a handler for didHide
  • Added a flag to identify if there is a tooltip visible
  • Added props for onHide and onShow
  • Updated readme file

@chirag04 let me know if it is ok to merge and how can we do the update of the npm library ;)

@enahum enahum requested a review from chirag04 February 8, 2018 13:39
ToolTip.ios.js Outdated
const ToolTipMenu = NativeModules.ToolTipMenu;
const RCTToolTipText = requireNativeComponent('RCTToolTipText', null);

export let isToolTipVisible = false;
Copy link
Owner

Choose a reason for hiding this comment

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

i would put this in the component and expose via ref. this.refs.tooltip.isToolTipVisible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the idea is to use it as an import.

if you have multiple tooltip components and you want to know if there is at least one of those components showing the tooltip without looping though all of them.

does it make sense?

Copy link
Owner

Choose a reason for hiding this comment

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

if you have multiple tooltip components and you want to know if there is at least one of those components showing the tooltip without looping though all of them.

That sounds like a state that should live in your application then. It's little weird to keep this global export in a library.idk

@chirag04
Copy link
Owner

chirag04 commented Feb 8, 2018

lgtm otherwise

@enahum
Copy link
Collaborator Author

enahum commented Feb 8, 2018

@chirag04 agree, I removed the isToolTipVisible completely

@chirag04
Copy link
Owner

chirag04 commented Feb 8, 2018

@enahum i guess update the readme also and merge.

@fungilation
Copy link

This is a much bigger refactor than my PR, which works for me up to and including RN 0.53. Looks like a nice cleanup, if everything still works the same!

@enahum
Copy link
Collaborator Author

enahum commented Feb 8, 2018

@fungilation yeap everything works the same ;)

@chirag04 readme updated and merging...

@enahum enahum merged commit b84a627 into master Feb 8, 2018
@enahum enahum deleted the update branch February 8, 2018 16:09
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.

3 participants