-
-
Notifications
You must be signed in to change notification settings - Fork 53
Change default label in <Tooltip /> #638
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
Merged
kodiakhq
merged 7 commits into
rad-ui:main
from
chromium-52:629/tooltip-remove-default-label
Dec 14, 2024
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
78154cd
Remove tooltip default label
chromium-52 469f381
Add tests for tooltip
chromium-52 7482572
Make label prop optional
chromium-52 e175a51
chore: fix wording in tooltip test
chromium-52 3d939cf
Add more tests for <Tooltip />
chromium-52 9054fb1
Verify rendered tooltip content in invalid label test
chromium-52 d594b6e
Merge branch 'main' into 629/tooltip-remove-default-label
kodiakhq[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import React from 'react'; | ||
| import { render, screen } from '@testing-library/react'; | ||
| import userEvent from '@testing-library/user-event'; | ||
| import Tooltip from '../Tooltip'; | ||
|
|
||
| describe('Tooltip', () => { | ||
| test('renders component', () => { | ||
| render(<Tooltip label='label'>Hover me</Tooltip>); | ||
| expect(screen.getByText('Hover me')).toBeInTheDocument(); | ||
| expect(screen.queryByText('label')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| describe('Labels', () => { | ||
| test('renders tooltip on hover and hides on unhover', async() => { | ||
| render(<Tooltip label='label'>Hover me</Tooltip>); | ||
| // tooltip is initially hidden | ||
| expect(screen.queryByText('label')).not.toBeInTheDocument(); | ||
| const text = screen.getByText('Hover me'); | ||
| // hover over the trigger text | ||
| await userEvent.hover(text); | ||
| // tooltip is visible now | ||
| expect(screen.getByText('label')).toBeInTheDocument(); | ||
| // unhover from the trigger text | ||
| await userEvent.unhover(text); | ||
| // tooltip is hidden again | ||
| expect(screen.queryByText('label')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('renders tooltip when label is not given', async() => { | ||
| render(<Tooltip>Hover me</Tooltip>); | ||
| // tooltip is initially hidden | ||
| expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); | ||
| const text = screen.getByText('Hover me'); | ||
| // hover over the trigger text | ||
| await userEvent.hover(text); | ||
| // tooltip is visible now | ||
| expect(screen.getByRole('dialog')).toBeInTheDocument(); | ||
| // unhover from the trigger text | ||
| await userEvent.unhover(text); | ||
| // tooltip is hidden again | ||
| expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| test('renders tooltip when label is of an invalid type', async() => { | ||
| // @ts-expect-error: label should be a string | ||
| const { rerender } = render(<Tooltip label={42}>Hover me</Tooltip>); | ||
| // hover over the trigger text | ||
| await userEvent.hover(screen.getByText('Hover me')); | ||
| // tooltip renders without throwing an error | ||
| expect(screen.getByText(42)).toBeInTheDocument(); | ||
|
|
||
| // @ts-expect-error:label should be a string | ||
| rerender(<Tooltip label={true}>Hover me</Tooltip>); | ||
| // hover over the trigger text | ||
| await userEvent.hover(screen.getByText('Hover me')); | ||
| // empty tooltip renders without throwing an error | ||
| expect(screen.getByRole('dialog')).toBeInTheDocument(); | ||
|
|
||
| // @ts-expect-error: label should be a string | ||
| rerender(<Tooltip label={null}>Hover me</Tooltip>); | ||
| // hover over the trigger text | ||
| await userEvent.hover(screen.getByText('Hover me')); | ||
| // empty tooltip renders without throwing an error | ||
| expect(screen.getByRole('dialog')).toBeInTheDocument(); | ||
| }); | ||
| }); | ||
| }); | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
awesome! great job on adding tests
following the above comment, let us treat label as an empty string
test for weird cases when say label is a number, boolean etc (you might have to write tests in js instead of ts for those tests)
Uh oh!
There was an error while loading. Please reload this page.
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.
Is this just because this is a public library and could be used in someone's JS project?
edit: and if so, shouldn't we be increasing the scope of
label's type inTooltipPropsto include other types in addition tostring?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.
Correct, that's the goal here
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.
No, so the label will always be a string, the component shouldn't crash and bring the entire app down when weird props are passed - we do the error handling inside the component so its smart enough to deal with such real life edge cases
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.
Ah then should I just be testing that the component renders the toooltip without any errors when non-string props are passed in?
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.
correct, it shouldn't break the component - say when bool or integers are passed (this doesn't happen in ts projects, but its a risk in js projects)