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: use radix tooltip #4797

Merged
merged 1 commit into from
Jan 17, 2024
Merged

feat: use radix tooltip #4797

merged 1 commit into from
Jan 17, 2024

Conversation

edgarkhanzadian
Copy link
Contributor

@edgarkhanzadian edgarkhanzadian commented Jan 11, 2024

Try out this version of Leather — Extension build, Test report

Closes #4506

tooltip.mov

@kyranjamie
Copy link
Collaborator

Not a big fan of the WithTooltip naming, seems kinda inconsistent with the rest of the components. Why not just Tooltip?

@@ -14,4 +14,34 @@ export const keyframes: CssKeyframes = {
transform: 'translateY(0)',
},
},
slideRightAndFade: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I just put up a PR in the mono repo, so I can add these there so you can remove them. I moved the other one.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fbwoolf
Copy link
Contributor

fbwoolf commented Jan 11, 2024

Agree about the naming here, and you might ref @kyranjamie's PR for the tabs bc I'm going to refactor the Select and DropdownMenu to follow that one. This one should follow same pattern...
#4798

@edgarkhanzadian
Copy link
Contributor Author

So what I ended up doing here is re-exporting the radix primitives with customized styles but also creating a separate wrapper component called BasicTooltip that i've applied in all of the cases in the repo (as we don't need much customization for a tooltip). Otherwise i would've ended up with a lot of repeatable code all over the repo with tooltip roots, contents, triggers, etc. Let me know if that works for us, otherwise i can just use the re-exported radix primitives instead of BasicTooltip

@edgarkhanzadian edgarkhanzadian marked this pull request as ready for review January 12, 2024 16:25
Copy link
Collaborator

@kyranjamie kyranjamie left a comment

Choose a reason for hiding this comment

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

Great refactor 💯

src/app/ui/components/tooltip/basic-tooltip.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@fbwoolf fbwoolf left a comment

Choose a reason for hiding this comment

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

Excellent! 🚀

@edgarkhanzadian edgarkhanzadian added this pull request to the merge queue Jan 17, 2024
Merged via the queue into dev with commit aa8a530 Jan 17, 2024
28 checks passed
@edgarkhanzadian edgarkhanzadian deleted the feat/use-radix-tooltip branch January 17, 2024 13:04
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.

Replace Tippy with Radix tooltip
3 participants