Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/components/ui/Tooltip/Tooltip.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ const COMPONENT_NAME = 'Tooltip';

type TooltipProps = {
children: React.ReactNode;
label: string;
label?: string;
placement?:
| 'top'
| 'bottom'
Expand All @@ -22,7 +22,7 @@ type TooltipProps = {
[key: string]: any;
};

const Tooltip = ({ children, label = 'hello', placement = 'top', ...props }: TooltipProps) => {
const Tooltip = ({ children, label = '', placement = 'top', ...props }: TooltipProps) => {
return (
<div>
<Popper
Expand Down
67 changes: 67 additions & 0 deletions src/components/ui/Tooltip/tests/Tooltip.test.tsx
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', () => {
Copy link
Collaborator

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)

Copy link
Contributor Author

@chromium-52 chromium-52 Dec 13, 2024

Choose a reason for hiding this comment

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

test for weird cases when say label is a number, boolean etc

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 in TooltipProps to include other types in addition to string?

Copy link
Collaborator

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?

Correct, that's the goal here

Copy link
Collaborator

Choose a reason for hiding this comment

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

edit: and if so, shouldn't we be increasing the scope of label's type in TooltipProps to include other types in addition to string?

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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?

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)

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();
});
});
});
Loading