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

ui/Tooltip - basic implementation of tooltip overlay #247

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Goondrious
Copy link
Collaborator

@Goondrious Goondrious commented Nov 8, 2021

I did a basic implementation for #224 and tried to keep it flexible, since there hadn't been a design discussion yet. I quickly looked at some existing packages, but they didn't seem compatible with the Hoverable/Button implementation.

I'm pretty new to typescript, react native and this project, so I'd be happy to refactor to conform to any standards that I missed.

Screen Shot 2021-11-08 at 5 26 31 PM
Screen Shot 2021-11-08 at 5 27 02 PM
Screen Shot 2021-11-08 at 5 26 55 PM
Screen Shot 2021-11-08 at 5 26 45 PM

Immediate TODOs or improvements would be:

  • [] more configurable style related props (e.g. color of background & text)
  • [] stay on screen
  • [] switch the usage from supplying a ref of the desired component to composition with built-in hover/touch/arbitrary trigger

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2021

CLA assistant check
All committers have signed the CLA.

@andymatuschak
Copy link
Owner

(I'm out of town this week but will get back to you next week—thank you!)

Copy link
Owner

@andymatuschak andymatuschak left a comment

Choose a reason for hiding this comment

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

Oh gosh, I'm terribly sorry that I lost track of this, @Goondrious. I took some time today to run through your changes.

You mentioned that this is your first attempt using TypeScript: an ambitious PR! :) Unfortunately, the TypeScript compiler had many complaints about the types in this patch. You can see them by running yarn build in the relevant module's subpath—or by using your editor's TypeScript integration. I went ahead and fixed most of the type errors, which involved changing the structure a bit in a few places. A few issues remain which require some discussion—see inline comments.

I also fixed a number of linter issues. That's my fault—there's no developer documentation for this project yet, so you had no way to know that existed!—but you can run lint checks with yarn lint in the root of the repository. Many of them can be automatically fixed with yarn lint:fix.

Thanks again for this patch and for your patience!

ViewStyle,
} from "react-native";

const styles = StyleSheet.create({
Copy link
Owner

Choose a reason for hiding this comment

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

You did the right thing by just taking a stab here. I'll come back through and match the art direction once we get everything squared away.

packages/ui/src/components/Tooltip.tsx Show resolved Hide resolved
packages/ui/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
packages/ui/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
packages/ui/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
packages/ui/src/components/Tooltip.tsx Outdated Show resolved Hide resolved
@Goondrious
Copy link
Collaborator Author

No worries @andymatuschak , thanks for the response.

My TypeScript experience has just been in passing, though now I've used it more in personal projects. I feel bad that you went through and did some of the basic tidying up, which I had purposefully left assuming that much would change through any design discussion. Sorry if it caused unnecessary effort/stress on your part. Thank you! I'll review your changes, as I'm sure it'll be a good learning experience. I was actually anticipating a blanket, "please fix type/lint errors".

Hoping to go through more specific comments later this week.

@Goondrious
Copy link
Collaborator Author

Apologies, I got caught up with some other work and holidays. Hopefully back to this soon.

@Goondrious
Copy link
Collaborator Author

@andymatuschak base checks pass. Can sort out styling, configurability and anything else. I'm also around for the next few weeks if there are other tasks you'd like help with.

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