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

Feat/tooltip #1088

Closed
wants to merge 7 commits into from
Closed

Feat/tooltip #1088

wants to merge 7 commits into from

Conversation

brunohkbx
Copy link
Collaborator

@brunohkbx brunohkbx commented May 24, 2019

Motivation

Add component Tooltip

Description

In this section, I'm going to highlight some key points of the implementation that I'm not sure if it's ok.

  1. I had to pass a prop onLongPress on L242 to prevent the children's touchable to respond to an onPress event right after the end of onLongPress

  2. I don't know why but I had to use a setTimeout with 500 ms to properly compute the Y position of the children on L126. It will get random values for Y without this setTimeout or with a smaller value.
    I've found the same problem on this implementation: https://github.com/react-native-training/react-native-elements/blob/master/src/tooltip/Tooltip.js#L145 and it's somehow related to measureInWindow returns an incorrect Y coord on Android facebook/react-native#19497

  3. The tooltip starts with opacity: 0 and then it changes to 0.9 when its measure gets calculated at L132. Should I have used animation to change this value?

TODOs

  • Fix tooltip coords when orientation change - 8ea66a8
    onLayout doesn't trigger on children component when the orientation changes, so I had to remove it.
  • Fix flow - b7bd00d
  • Test on IOS - It works!! 😃
  • Add normal and tsx tests - b50fadd
  • Add docs - 8875998
  • Implement tooltip placement prop ?

Material Design specs

  1. Don't crop Tooltip on the left side
    left

  2. Don't crop Tooltip on the right side
    right

  3. Don't crop Tooltip on the bottom
    bottom

  4. Landscape mode
    landscape

Test plan

  1. Download this branch
  2. Run example app
  3. Open Appbar or Toggle Button section and hold press on any icon with Tooltip.

References

Screenshots

tooltip1

tooltip1

@callstack-bot
Copy link

callstack-bot commented May 24, 2019

Hey @brunohkbx, thank you for your pull request 🤗. The documentation from this branch can be viewed here. Please remember to update Typescript types if you changed API.

@brunohkbx brunohkbx requested review from satya164, jaulz and Trancever May 24, 2019 04:55
@brunohkbx brunohkbx marked this pull request as ready for review May 27, 2019 11:42
@Trancever Trancever closed this Jun 3, 2019
@Trancever Trancever reopened this Jun 3, 2019
@andresmtz98
Copy link

Any update on this?

@brunohkbx
Copy link
Collaborator Author

@andresmtz98 I'm waiting for @Trancever's review.

@Trancever
Copy link
Contributor

@brunohkbx I resolved all the conflicts + added support for typescript locally but I can't push it, because you created PR from your fork. I am pretty sure you have a write access, so please use main repo instead of fork next time :D

Could you open new PR from brunohkbx-feat/tooltip branch from main repo so the contribution will be counted as yours?

@brunohkbx
Copy link
Collaborator Author

I didn't realize that I was working from the forked repository. 😞
Thanks for the rebase @Trancever.
Done #1185

@brunohkbx brunohkbx closed this Jul 5, 2019
@brunohkbx brunohkbx deleted the feat/tooltip branch July 5, 2019 14:23
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