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

TooltipButton: Converted component to TypeScript #2317

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

shleewhite
Copy link
Contributor

@shleewhite shleewhite commented Aug 5, 2024

📌 Summary

Converts TooltipButton to typescript

🔗 External links

Jira ticket: HDS-2718


👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Aug 5, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Aug 5, 2024 6:11pm
hds-website ✅ Ready (Inspect) Visit Preview Aug 5, 2024 6:11pm

offset: this.args.offset,
showOnCreate: true,
// takes array of 2 numbers (skidding, distance): array(0, 10)
offset: this.args.offset ? this.args.offset : [0, 10],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: turns out the default if offset is not provided is [0,10] not [0,0]

https://github.com/atomiks/tippyjs/blob/ad85f6feb79cf6c5853c43bf1b2a50c4fa98e7a1/src/props.ts#L44

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! 👏


export interface HdsTooltipIndexSignature {
Args: {
extraTippyOptions: Omit<TippyProps, 'placement' | 'offset'>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: since placement and offset will always get overwritten on line 53, I figure we just omit them from the type to avoid confusion.

Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

All good for me 👍
But wait for @alex-ju approval before merging ;)

offset: this.args.offset,
showOnCreate: true,
// takes array of 2 numbers (skidding, distance): array(0, 10)
offset: this.args.offset ? this.args.offset : [0, 10],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch! 👏

@shleewhite shleewhite merged commit 70aeec3 into main Aug 6, 2024
16 checks passed
@shleewhite shleewhite deleted the hds-2718/ts-tooltip branch August 6, 2024 13:51
@hashibot-hds hashibot-hds mentioned this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants