-
-
Notifications
You must be signed in to change notification settings - Fork 54
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
base: master
Are you sure you want to change the base?
Conversation
(I'm out of town this week but will get back to you next week—thank you!) |
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.
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({ |
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.
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.
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. |
Apologies, I got caught up with some other work and holidays. Hopefully back to this soon. |
@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. |
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.
Immediate TODOs or improvements would be:
ref
of the desired component to composition with built-in hover/touch/arbitrary trigger